GNOME Bugzilla – Bug 679706
Remove most of 'is UnattendedInstaller' checks
Last modified: 2016-03-31 13:58:41 UTC
Code was getting full of these checks and IMO getting hard to follow.
Created attachment 218470 [details] [review] Remove most of 'is UnattendedInstaller' checks We now make maximum use of virtual functions to relieve the users of InstallerMedia to check if the instance is of subclass UnattendedInstaller before accessing UnattendedInstaller members. For this to happen, some of the functions were moved to base InstallerMedia class and were appropriately renamed/modified.
Could you split that into roughly one patch per new virtual method? This would make it much easier to review.
(In reply to comment #2) > Could you split that into roughly one patch per new virtual method? This would > make it much easier to review. Divding part is easy, coming-up with new descriptive log messages is difficult. :)
Something like "Add virtual UnattendedInstall::foo This allows to remove some behaviour conditioned by runtime type checks" For each commit may be enough
Created attachment 218512 [details] [review] InstallerMedia tells if setup is needed No need to use 'is UnattendedInstaller' check for this now.
Created attachment 218513 [details] [review] UnattendedInstaller.set_direct_boot_params -> InstallerMedia Move set_direct_boot_params from UnattendedInstaller to parent InstallerMedia so that users of this function don't need type checks and casting to be able to use it.
Created attachment 218514 [details] [review] UnattendedInstaller.setup -> InstallerMedia.prepare_for_installation Move setup from UnattendedInstaller to parent InstallerMedia as a virtual function so that users of this function don't need type checks and casting to be able to use it. Also give it a more specific/descriptive name.
Created attachment 218515 [details] [review] Move installer disks setup to InstallerMedia Let InstallerMedia and its subclasses handle setup of source and unattended disk configuration in the domain config.
Created attachment 218516 [details] [review] Move SPICE password setup to InstallerMedia Let InstallerMedia and its subclasses handle setup of SPICE password in the domain config.
Created attachment 218517 [details] [review] UnattendedInstaller.populate_setup_vbox -> InstallerMedia Move populate_setup_vbox from UnattendedInstaller to parent InstallerMedia so that users of this function don't need type checks and casting to be able to use it.
Created attachment 218518 [details] [review] UnattendedInstaller.check_needed_info -> InstallerMedia Move check_needed_info from UnattendedInstaller to parent InstallerMedia so that users of this function don't need type checks and casting to be able to use it.
Created attachment 218519 [details] [review] Add InstallerMedia.get_vm_properties() Let InstallerMedia and its subclasses provide list of VM properties.
Comment on attachment 218512 [details] [review] InstallerMedia tells if setup is needed >From 4172d978200aa9a27d03a83e5459cc20ea1f04d7 Mon Sep 17 00:00:00 2001 >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> >Date: Wed, 11 Jul 2012 10:53:00 +0300 >Subject: [PATCH] InstallerMedia tells if setup is needed > >No need to use 'is UnattendedInstaller' check for this now. > >https://bugzilla.gnome.org/show_bug.cgi?id=679706 >--- > src/installer-media.vala | 1 + > src/unattended-installer.vala | 1 + > src/wizard.vala | 5 ++--- > 3 files changed, 4 insertions(+), 3 deletions(-) > >diff --git a/src/installer-media.vala b/src/installer-media.vala >index fa848c5..3af4b27 100644 >--- a/src/installer-media.vala >+++ b/src/installer-media.vala >@@ -12,6 +12,7 @@ > public string mount_point; > public bool from_image; > >+ public bool setup_required { get; protected set; default = false; } "setup_required" is about showing/not showing the wizard setup page right? If so, a more descriptive name would be useful I think. show_setup_page maybe? apart from this comment, patch looks good.
Comment on attachment 218513 [details] [review] UnattendedInstaller.set_direct_boot_params -> InstallerMedia Usual reluctance with having something very specific to FedoraInstaller leak as a no-op vfunc its great-great parent...
Looking at this series makes me wonder if it wouldn't be cleaner to have an UnattendedVmConfigurator deriving from VmConfigurator with an UnattendedInstaller member. Is this something you have considered and rejected while doing this work?
Comment on attachment 218517 [details] [review] UnattendedInstaller.populate_setup_vbox -> InstallerMedia still not a big fan of empty vfuncs ;)
Comment on attachment 218516 [details] [review] Move SPICE password setup to InstallerMedia too bad we cannot do a more generic setup_graphics_config(DomainGraphics graphics) method (no set_password method on DomainGraphics)
Comment on attachment 218515 [details] [review] Move installer disks setup to InstallerMedia Looks good. This code could probably be further simplified by getting rid of get_unattended_disk_config and moving that into setup_domain_config
(In reply to comment #13) > (From update of attachment 218512 [details] [review]) CUT > >diff --git a/src/installer-media.vala b/src/installer-media.vala > >index fa848c5..3af4b27 100644 > >--- a/src/installer-media.vala > >+++ b/src/installer-media.vala > >@@ -12,6 +12,7 @@ > > public string mount_point; > > public bool from_image; > > > >+ public bool setup_required { get; protected set; default = false; } > > "setup_required" is about showing/not showing the wizard setup page right? If > so, a more descriptive name would be useful I think. show_setup_page maybe? > apart from this comment, patch looks good. Yeah, more like 'user_input_required'.
(In reply to comment #14) > (From update of attachment 218513 [details] [review]) > Usual reluctance with having something very specific to FedoraInstaller leak as > a no-op vfunc its great-great parent... More like its currently only used by FedoraInstaller and is one of the parts that we'd need to make more generic as part of ditching all UnattendedInstaller's subclasses in favor of new libosinfo installer script API. (In reply to comment #15) > Looking at this series makes me wonder if it wouldn't be cleaner to have an > UnattendedVmConfigurator deriving from VmConfigurator with an > UnattendedInstaller member. Is this something you have considered and rejected > while doing this work? I didn't cause VMConfigurator has now only static utility methods so inheritance wouldn't make much sense or even work. (In reply to comment #16) > (From update of attachment 218517 [details] [review]) > still not a big fan of empty vfuncs ;) Me neither but AFAICT its its either empty vfuncs or those checks/casts we are removing here. I'd very much prefer the former as long as we keep those vfuncs generic look generic enough. (In reply to comment #17) > (From update of attachment 218516 [details] [review]) > too bad we cannot do a more generic setup_graphics_config(DomainGraphics > graphics) method (no set_password method on DomainGraphics) Yeah, I think I actually tried it and compiler gave me the bad news. (In reply to comment #18) > (From update of attachment 218515 [details] [review]) > Looks good. > This code could probably be further simplified by getting rid of > get_unattended_disk_config and moving that into setup_domain_config True, I'll look into that.
(In reply to comment #20) > (In reply to comment #18) > > (From update of attachment 218515 [details] [review] [details]) > > Looks good. > > This code could probably be further simplified by getting rid of > > get_unattended_disk_config and moving that into setup_domain_config > > True, I'll look into that. Actually no and now that I looked at the code, I remember thats what I tried at first. Thing is that get_unattended_disk_config is overridden by WindowsInstaller to get a hold of the disk config and modify it before its added to the domain config.
Created attachment 218573 [details] [review] InstallerMedia tells if setup is needed v2: More descriptive name for the prop.
Attachment 218513 [details] pushed as 8d300ce - UnattendedInstaller.set_direct_boot_params -> InstallerMedia Attachment 218514 [details] pushed as 7626371 - UnattendedInstaller.setup -> InstallerMedia.prepare_for_installation Attachment 218515 [details] pushed as d57cdbb - Move installer disks setup to InstallerMedia Attachment 218516 [details] pushed as 27050e8 - Move SPICE password setup to InstallerMedia Attachment 218517 [details] pushed as f62e971 - UnattendedInstaller.populate_setup_vbox -> InstallerMedia Attachment 218518 [details] pushed as e788db1 - UnattendedInstaller.check_needed_info -> InstallerMedia Attachment 218519 [details] pushed as d7f0646 - Add InstallerMedia.get_vm_properties()
Created attachment 218578 [details] [review] InstallerMedia tells if setup is needed V3: Rebase (Non-automated) on current git master.
(In reply to comment #21) > Actually no and now that I looked at the code, I remember thats what I tried at > first. Thing is that get_unattended_disk_config is overridden by > WindowsInstaller to get a hold of the disk config and modify it before its > added to the domain config. I saw that too before commenting, but this didn't seem impossible to address... One way possible way may be to have an unattended_disk_type member.
(In reply to comment #24) > Created an attachment (id=218578) [details] [review] > InstallerMedia tells if setup is needed > > V3: Rebase (Non-automated) on current git master. With 'user_input_required', we don't know it's something needed at the wizard setup step.
(In reply to comment #26) > (In reply to comment #24) > > Created an attachment (id=218578) [details] [review] [details] [review] > > InstallerMedia tells if setup is needed > > > > V3: Rebase (Non-automated) on current git master. > > With 'user_input_required', we don't know it's something needed at the wizard > setup step. I don't think there is any need for InstallerMedia hierarchy to know if there is a wizard and that it has a setup step/page. They just report if user input is needed for vm creation or not and if it is, wizard asks them to give it the widgets we can show to user for getting this input.
(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #24) > > > Created an attachment (id=218578) [details] [review] [details] [review] [details] [review] > > > InstallerMedia tells if setup is needed > > > > > > V3: Rebase (Non-automated) on current git master. > > > > With 'user_input_required', we don't know it's something needed at the wizard > > setup step. > > I don't think there is any need for InstallerMedia hierarchy to know if there > is a wizard and that it has a setup step/page. They just report if user input > is needed for vm creation or not and if it is, wizard asks them to give it the > widgets we can show to user for getting this input. needs_user_setup? I find user_input_required very unspecific, that's why I'm trying to find something else.
Review of attachment 218578 [details] [review]: ::: src/unattended-installer.vala @@ +10,3 @@ + public override bool user_input_required { + get { + return !live; // No setup required by live media s/live media/live or unknown media/ Do we instantiate an UnattendedInstaller object for live/unknown media? Or should this return true; and InstallerMedia::user_input_required be changed to return !live?
(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #26) > > > (In reply to comment #24) > > > > Created an attachment (id=218578) [details] [review] [details] [review] [details] [review] [details] [review] > > > > InstallerMedia tells if setup is needed > > > > > > > > V3: Rebase (Non-automated) on current git master. > > > > > > With 'user_input_required', we don't know it's something needed at the wizard > > > setup step. > > > > I don't think there is any need for InstallerMedia hierarchy to know if there > > is a wizard and that it has a setup step/page. They just report if user input > > is needed for vm creation or not and if it is, wizard asks them to give it the > > widgets we can show to user for getting this input. > > needs_user_setup? I find user_input_required very unspecific, that's why I'm > trying to find something else. As discussed on IRC, I'll go with needs_user_input_for_vm_creation for now. Someone can change it later if they can come-up with short name. (In reply to comment #29) > Review of attachment 218578 [details] [review]: > > ::: src/unattended-installer.vala > @@ +10,3 @@ > + public override bool user_input_required { > + get { > + return !live; // No setup required by live media > > s/live media/live or unknown media/ > > Do we instantiate an UnattendedInstaller object for live/unknown media? Or > should this return true; and InstallerMedia::user_input_required be changed to > return !live? We don't instantiate UnattendedInstaller object for unknown but we do for live media. We don't have any user input/setup unless its a unattended installer and media is not live, hence this needs to be here and not baseclass.
> > ::: src/unattended-installer.vala > > @@ +10,3 @@ > > + public override bool user_input_required { > > + get { > > + return !live; // No setup required by live media > > > > s/live media/live or unknown media/ > > > > Do we instantiate an UnattendedInstaller object for live/unknown media? Or > > should this return true; and InstallerMedia::user_input_required be changed to > > return !live? > > We don't instantiate UnattendedInstaller object for unknown but we do for live > media. We don't have any user input/setup unless its a unattended installer and > media is not live, hence this needs to be here and not baseclass. Ok, your initial comment was good then, but I'd complete it with (unknown medias are not UnattendedInstaller instances), even though this may be a bit cryptic if you don't know what 'live' means.
Attachment 218578 [details] pushed as a902f81 - InstallerMedia tells if setup is needed I believe the last patch that I just pushed addresses the remaining concerns so pushing it already. Let me know if thats not the case and I'll address any remaining issues.