GNOME Bugzilla – Bug 690775
regression: CDROM device gets removed after successful install
Last modified: 2016-03-31 13:55:36 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.
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.
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.
Created attachment 232283 [details] [review] Move remove_disk_from_domain_config to UnattendedInstaller InstallerMedia no longer uses it after the previous commit.
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.
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.
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.
Created attachment 232285 [details] [review] Move remove_disk_from_domain_config to UnattendedInstaller InstallerMedia no longer uses it after the previous commit.
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.
(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?
(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.
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?
(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.
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.
(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.
(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().
(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'...
(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().
(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.
(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.
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