GNOME Bugzilla – Bug 690536
Clean-up installer resources after installation is complete
Last modified: 2016-03-31 13:55:25 UTC
Attaching patches to: * Delete the unattended disk, kernel & initrd files after installation. * Remove the unattended disk and CD-ROM from domain configuration after installation. * Tidy-up the installer source code a bit.
Created attachment 231945 [details] [review] installer: Remove redundant error domain This has been redundant since 2b22dfe but compiler didn't warn about it since it was declared 'public'.
Created attachment 231946 [details] [review] installer: Remove redundant get_preferred_language() argument
Created attachment 231947 [details] [review] installer: Minor coding-style fixes - Move private get_preferred_language() below public methods. - Don't exceed 120 chars.
Created attachment 231948 [details] [review] installer: UnattendedTextFile -> UnattendedScriptFile Rename to reflect the role of this class better after recent changes.
Created attachment 231949 [details] [review] installer: Move Unattended*File to separate module Move UnattendedFile hierarchy to separate module.
Created attachment 231950 [details] [review] installer: Don't lose pointers to unattended resources Don't lose pointers to unattended disk, kernel and initrd files on UI exit.
Created attachment 231951 [details] [review] installer: Remove unattended disk from config after install Remove unattended disk from domain configuration after installation is complete.
Created attachment 231952 [details] [review] installer: Remove CDROM from config after installation Remove CDROM disk from domain configuration after installation is complete.
Created attachment 231953 [details] [review] installer: Clean-up resources after installation This currently only applies to unattended installations and it means deletion of unattended disk, kernel and initrd files.
Review of attachment 231945 [details] [review]: ack
Review of attachment 231946 [details] [review]: ack
Review of attachment 231947 [details] [review]: ack
Review of attachment 231948 [details] [review]: ack
Review of attachment 231949 [details] [review]: ack
Review of attachment 231950 [details] [review]: ack
Review of attachment 231951 [details] [review]: What changed and made "some OSs need express install data after (e.g during first-boot setup)" not true anymore? Or am i missing something?
Review of attachment 231952 [details] [review]: ack
Review of attachment 231953 [details] [review]: ack
Review of attachment 231951 [details] [review]: Oh, I only noticed that with Fedora in the past but I have tested these patches against Fedora18 so at least >=F18 doesn't break with this. Its very much possible that it was something wrong in the script that Fedora didn't accept and greeted user with first-boot setup. I'll now test these changes against F17 to be sure..
(In reply to comment #19) > Review of attachment 231951 [details] [review]: > > Oh, I only noticed that with Fedora in the past but I have tested these patches > against Fedora18 so at least >=F18 doesn't break with this. Its very much > possible that it was something wrong in the script that Fedora didn't accept > and greeted user with first-boot setup. I'll now test these changes against F17 > to be sure.. I just did a successful F17 install with these patches. Now trying XP just to be sure.
If that works, ack for that patch too.
(In reply to comment #21) > If that works, ack for that patch too. Yup, XP installed fine too.
Attachment 231945 [details] pushed as fc68530 - installer: Remove redundant error domain Attachment 231946 [details] pushed as 3f0d677 - installer: Remove redundant get_preferred_language() argument Attachment 231947 [details] pushed as 1588acc - installer: Minor coding-style fixes Attachment 231948 [details] pushed as d211632 - installer: UnattendedTextFile -> UnattendedScriptFile Attachment 231949 [details] pushed as ab2d3a2 - installer: Move Unattended*File to separate module Attachment 231950 [details] pushed as 446be20 - installer: Don't lose pointers to unattended resources Attachment 231951 [details] pushed as 1f32090 - installer: Remove unattended disk from config after install Attachment 231952 [details] pushed as e52fcad - installer: Remove CDROM from config after installation Attachment 231953 [details] pushed as 4e1c605 - installer: Clean-up resources after installation
Review of attachment 231952 [details] [review]: ::: src/installer-media.vala @@ +136,3 @@ + } + + domain.set_devices (devices); This removes the CDROM device from the libvirt config, while what we want is to eject the CDROM image from the CDROM device but keep this device. This breaks 'f34ce3b Add support for changing the CDROM iso'
(In reply to comment #24) > Review of attachment 231952 [details] [review]: > > ::: src/installer-media.vala > @@ +136,3 @@ > + } > + > + domain.set_devices (devices); > > This removes the CDROM device from the libvirt config, while what we want is to > eject the CDROM image from the CDROM device but keep this device. This breaks > 'f34ce3b Add support for changing the CDROM iso' Although it was patches in this bug that introduced this issue, the new problem is not really this bug (i-e we do now clean the resources after installation) so please open another bug for it.