After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 690775 - regression: CDROM device gets removed after successful install
regression: CDROM device gets removed after successful install
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2012-12-27 15:37 UTC by Christophe Fergeau
Modified: 2016-03-31 13:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Pass GVir.Domain to VMConfigurator::post_install_setup (3.12 KB, patch)
2012-12-27 15:37 UTC, Christophe Fergeau
reviewed Details | Review
Do not remove CDROM device after install (2.24 KB, patch)
2012-12-27 15:37 UTC, Christophe Fergeau
reviewed Details | Review
Move remove_disk_from_domain_config to UnattendedInstaller (2.20 KB, patch)
2012-12-27 15:37 UTC, Christophe Fergeau
none Details | Review
Do not remove CDROM device after install (3.27 KB, patch)
2012-12-27 16:16 UTC, Christophe Fergeau
committed Details | Review
Move remove_disk_from_domain_config to UnattendedInstaller (2.20 KB, patch)
2012-12-27 16:16 UTC, Christophe Fergeau
committed Details | Review
installer-media: Keep CD-ROM device after install (2.26 KB, patch)
2012-12-27 19:54 UTC, Zeeshan Ali
none Details | Review

Description Christophe Fergeau 2012-12-27 15:37:37 UTC
commit e52fcad "installer: Remove CDROM from config after installation"
removes the CDROM device after a successful installation rather than ejecting
the installation CD image from the CDROM device. This means that Boxes will
provide the user with VMs without any CDROM device. This in turn makes
the work from commit f34ce3b "Add support for changing the CDROM iso"
quite useless.

These patches change it so that the CDROM device is not removed, but the
CD image is ejected instead. I'm currently testing these with a winxp
installation.
Comment 1 Christophe Fergeau 2012-12-27 15:37:40 UTC
Created attachment 232281 [details] [review]
Pass GVir.Domain to VMConfigurator::post_install_setup

We are currently only passing a GVirConfig.Domain object to
VMConfigurator::post_install_setup, but in the follow-up patches
we'll need access to a GVir.Domain instance, so let's pass that
instead.
Comment 2 Christophe Fergeau 2012-12-27 15:37:43 UTC
Created attachment 232282 [details] [review]
Do not remove CDROM device after install

Once an install is done, we do not want to remove the CDROM device
from the current VM as this would mean the user cannot use CD
images with that VM. This is a regression that was introduced
in commit 'e52fcad installer: Remove CDROM from config after installation'
and which breaks the work that was done as part of
'f34ce3b Add support for changing the CDROM iso'

This commit only ejects  the current CD from the CDROM device, but
keeps the device around for future use.
Comment 3 Christophe Fergeau 2012-12-27 15:37:45 UTC
Created attachment 232283 [details] [review]
Move remove_disk_from_domain_config to UnattendedInstaller

InstallerMedia no longer uses it after the previous commit.
Comment 4 Zeeshan Ali 2012-12-27 15:43:36 UTC
Review of attachment 232281 [details] [review]:

This beats one of the main point of having the VMConfigurator separate from VMCreator. Also now this one function looks very much out of the place as rest of the API operates only on the configuration, not the domain.
Comment 5 Zeeshan Ali 2012-12-27 15:48:03 UTC
Review of attachment 232282 [details] [review]:

::: src/vm-configurator.vala
@@ +125,3 @@
+                    // eject CDROM contain in the CD drive as it will not be useful after installation
+                    disk.set_source ("");
+                    domain.update_device (disk, GVir.DomainUpdateDeviceFlags.CONFIG);

The domain is shutoff at this moment so there is no need to do anything to the domain itself, only updating the configuration is enough. Hence:

1. No need from the previous patch.
2. This could/should still be done by installer-media.
Comment 6 Christophe Fergeau 2012-12-27 16:16:24 UTC
Created attachment 232284 [details] [review]
Do not remove CDROM device after install

Once an install is done, we do not want to remove the CDROM device
from the current VM as this would mean the user cannot use CD
images with that VM. This is a regression that was introduced
in commit 'e52fcad installer: Remove CDROM from config after installation'
and which breaks the work that was done as part of
'f34ce3b Add support for changing the CDROM iso'

This commit only ejects  the current CD from the CDROM device, but
keeps the device around for future use.
Comment 7 Christophe Fergeau 2012-12-27 16:16:27 UTC
Created attachment 232285 [details] [review]
Move remove_disk_from_domain_config to UnattendedInstaller

InstallerMedia no longer uses it after the previous commit.
Comment 8 Zeeshan Ali 2012-12-27 19:54:16 UTC
Created attachment 232295 [details] [review]
installer-media: Keep CD-ROM device after install

Once an install is done, we do not want to remove the CD-ROM device from
the current VM as this would mean the user cannot use CD images with that
VM. This is a regression that was introduced in commit e52fcad
'installer: Remove CD-ROM from config after installation' and which breaks
the work that was done as part of commit f34ce3b 'Add support for
changing the CD-ROM iso'.

This commit only ejects  the current CD from the CD-ROM device, but
keeps the device around for future use.
Comment 9 Zeeshan Ali 2012-12-27 19:55:28 UTC
(In reply to comment #8)
> Created an attachment (id=232295) [details] [review]
> installer-media: Keep CD-ROM device after install

How about this less intrusive solution instead?
Comment 10 Christophe Fergeau 2012-12-27 20:24:23 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > Created an attachment (id=232295) [details] [review] [details] [review]
> > installer-media: Keep CD-ROM device after install
> 
> How about this less intrusive solution instead?

Really not a fan of having remove_disk_from_domain_config have 2 widely different behaviours depending on its 3rd argument. The 'intrusive' solution net result is only +6 new lines of code as it's mostly code movement.
Comment 11 Zeeshan Ali 2012-12-27 20:52:38 UTC
Review of attachment 232285 [details] [review]:

I think your last two patches break removal of unattended disk after install.

::: src/unattended-installer.vala
@@ +722,3 @@
     }
+
+    private void remove_disk_from_domain_config (Domain domain, string disk_path) {

where is this being called from now?
Comment 12 Zeeshan Ali 2012-12-27 20:55:12 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Created an attachment (id=232295) [details] [review] [details] [review] [details] [review]
> > > installer-media: Keep CD-ROM device after install
> > 
> > How about this less intrusive solution instead?
> 
> Really not a fan of having remove_disk_from_domain_config have 2 widely
> different behaviours depending on its 3rd argument.

I don't see it as widely different behavior: removing media vs removing removing disk.

A better/cleaner solution would be to have separate functions (this part is covered by your patches) that share the code for finding the device and setting the modified device list back.
Comment 13 Christophe Fergeau 2012-12-27 21:08:54 UTC
Review of attachment 232285 [details] [review]:

::: src/unattended-installer.vala
@@ +722,3 @@
     }
+
+    private void remove_disk_from_domain_config (Domain domain, string disk_path) {

Same place as before, UnattendedInstaller::setup_post_install_domain_config
It used to be called twice, once for CDROM, once for unattended floppy, now it's only called once for the floppy.
Comment 14 Christophe Fergeau 2012-12-27 21:11:52 UTC
(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > Created an attachment (id=232295) [details] [review] [details] [review] [details] [review] [details] [review]
> > > > installer-media: Keep CD-ROM device after install
> > > 
> > > How about this less intrusive solution instead?
> > 
> > Really not a fan of having remove_disk_from_domain_config have 2 widely
> > different behaviours depending on its 3rd argument.
> 
> I don't see it as widely different behavior: removing media vs removing
> removing disk.
> 

We are talking about 'Ejecting a CD from the drive' VS 'Physically removing the CD reader from the computer', the code to achieve that is surprisingly similar, but the end result is widely different.



> A better/cleaner solution would be to have separate functions (this part is
> covered by your patches) that share the code for finding the device and setting
> the modified device list back.

If that's what is annoying you, my patches are not adding one such loop, but are reusing an existing one.
Comment 15 Zeeshan Ali 2012-12-27 21:56:57 UTC
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (In reply to comment #8)
> > > > > Created an attachment (id=232295) [details] [review] [details] [review] [details] [review] [details] [review] [details] [review]
> > > > > installer-media: Keep CD-ROM device after install
> > > > 
> > > > How about this less intrusive solution instead?
> > > 
> > > Really not a fan of having remove_disk_from_domain_config have 2 widely
> > > different behaviours depending on its 3rd argument.
> > 
> > I don't see it as widely different behavior: removing media vs removing
> > removing disk.
> > 
> 
> We are talking about 'Ejecting a CD from the drive' VS 'Physically removing the
> CD reader from the computer', the code to achieve that is surprisingly similar,
> but the end result is widely different.

IMO the boolean param and its simple usage tells the reader about the diff but yes, I agree that its not at all ideal. 

> > A better/cleaner solution would be to have separate functions (this part is
> > covered by your patches) that share the code for finding the device and setting
> > the modified device list back.
> 
> If that's what is annoying you, my patches are not adding one such loop, but
> are reusing an existing one.

You are adding a new eject_cdrom_media() function thats indendent of remove_disk_from_domain_config().
Comment 16 Christophe Fergeau 2012-12-27 22:09:15 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > 
> > If that's what is annoying you, my patches are not adding one such loop, but
> > are reusing an existing one.
> 
> You are adding a new eject_cdrom_media() function thats indendent of
> remove_disk_from_domain_config().

And I'm removing one from VMConfigurator::post_install_setup. If that's such an issue, you shouldn't have duplicated that loop in 1f32090 'installer: Remove unattended disk from config after install'...
Comment 17 Zeeshan Ali 2012-12-27 22:10:37 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (In reply to comment #10)
> > > > (In reply to comment #9)
> > > > > (In reply to comment #8)
> > > > > > Created an attachment (id=232295) [details] [review] [details] [review] [details] [review] [details] [review] [details] [review] [details] [review]
> > > > > > installer-media: Keep CD-ROM device after install
> > > > > 
> > > > > How about this less intrusive solution instead?
> > > > 
> > > > Really not a fan of having remove_disk_from_domain_config have 2 widely
> > > > different behaviours depending on its 3rd argument.
> > > 
> > > I don't see it as widely different behavior: removing media vs removing
> > > removing disk.
> > > 
> > 
> > We are talking about 'Ejecting a CD from the drive' VS 'Physically removing the
> > CD reader from the computer', the code to achieve that is surprisingly similar,
> > but the end result is widely different.
> 
> IMO the boolean param and its simple usage tells the reader about the diff but
> yes, I agree that its not at all ideal. 
> 
> > > A better/cleaner solution would be to have separate functions (this part is
> > > covered by your patches) that share the code for finding the device and setting
> > > the modified device list back.
> > 
> > If that's what is annoying you, my patches are not adding one such loop, but
> > are reusing an existing one.
> 
> You are adding a new eject_cdrom_media() function thats indendent of
> remove_disk_from_domain_config().

However, feel free to push your patches but please add a FIXME note above remove_disk_from_domain_config() saying it should share code with eject_cdrom_media().
Comment 18 Zeeshan Ali 2012-12-27 22:12:01 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > 
> > > If that's what is annoying you, my patches are not adding one such loop, but
> > > are reusing an existing one.
> > 
> > You are adding a new eject_cdrom_media() function thats indendent of
> > remove_disk_from_domain_config().
> 
> And I'm removing one from VMConfigurator::post_install_setup. If that's such an
> issue, you shouldn't have duplicated that loop in 1f32090 'installer: Remove
> unattended disk from config after install'...

I shouldn't have, yes but nobody pointed that out in the review so I didn't think of that. As I explained in the last comment, its not a blocker issue.
Comment 19 Zeeshan Ali 2012-12-27 22:16:26 UTC
(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > 
> > > > If that's what is annoying you, my patches are not adding one such loop, but
> > > > are reusing an existing one.
> > > 
> > > You are adding a new eject_cdrom_media() function thats indendent of
> > > remove_disk_from_domain_config().
> > 
> > And I'm removing one from VMConfigurator::post_install_setup. If that's such an
> > issue, you shouldn't have duplicated that loop in 1f32090 'installer: Remove
> > unattended disk from config after install'...
> 
> I shouldn't have, yes but nobody pointed that out in the review so I didn't
> think of that. As I explained in the last comment, its not a blocker issue.

To be clear, this is an ACK for your last two patches but not 'Pass GVir.Domain to VMConfigurator::post_install_setup' for the reasons I mentioned in its review.
Comment 20 Christophe Fergeau 2012-12-28 09:26:10 UTC
Attachment 232284 [details] pushed as 4de5899 - Do not remove CDROM device after install
Attachment 232285 [details] pushed as 26423c5 - Move remove_disk_from_domain_config to UnattendedInstaller