GNOME Bugzilla – Bug 691546
Automatically install QXL drivers for Windows XP
Last modified: 2016-03-31 14:00:54 UTC
boxes is kinda useless without QXL drivers, so our express installers should install it automatically.
Just FYI, I'm working on this..
So in the end I failed to get this working in a nice/acceptable way. :( Here are the approaches I took: * Install QXL driver during pre-installation just like the viostor driver. Windows XP and 7 simply ignore the QXL and there is no indication in the several installation log files about qxl. It only needed changes in libosinfo: https://gitorious.org/libosinfo/libosinfo/commits/preinst-all-virtio * Install QXL driver during post-installation: * using the all-in-one spice-guest-tools-0.3.exe . This is the only one I was successful in getting to work but Boxes changes requires are ugly. More details in the commit logs: https://gitorious.org/libosinfo/libosinfo/commits/posinst-drivers https://gitorious.org/gnome-boxes/gnome-boxes/commits/postinstall-drivers * using individual driver files put in the right place on the unattended disk and informing windows about the location of the drivers from unattended file. I failed to get windows XP (didn't try with windows 7) to install the drivers this way either. Just like the first approach above, the drivers are simply ignored and nothing in the logs that could give any clues. This approach should work according to these unofficial docs: http://unattended.msfn.org/unattended.xp/view/web/34/ Here are the commits: https://gitorious.org/libosinfo/libosinfo/commits/posinst-drivers-only https://gitorious.org/gnome-boxes/gnome-boxes/commits/postinstall-drivers-w/o-disk2 Suggestions on how to proceed further welcome!
Maybe just put spice-guest-tools-0.3.exe on the desktop after install? With a label that says you need to run it?
10:35 <@teuf> I'm quite sure we should even be able to cherry pick which drivers to install/not install 10:37 <@teuf> it seems the ugly changes zeenix is talking about would be avoided by generating an iso instead of a fat image 10:44 * teuf wonders if it would be possible to cheat using target or address libvirt disk nodes A few notes from my IRC log
Created attachment 234574 [details] [review] installer: Get rid of get_unattended_disk_info() method Keeping get_unattended_disk_info() separate from get_unattended_disk_config wasn't really helping readability much already and more so with the changes to follow.
Created attachment 234575 [details] [review] installer: Set bus on unattended disk config
Created attachment 234576 [details] [review] installer: Update progress on error to setup driver
Created attachment 234577 [details] [review] installer: Add a secondary unattended disk In the following patches, we are going to add support for post-installation drivers and the first one to come from libosinfo is going to take 13M more. Its time to add a secondary disk in ISO format that we create on demand at runtime.
Created attachment 234578 [details] [review] installer: Add get_post_installer_scripts() Add method to fetch scripts that can install drivers at the end of OS installation.
Created attachment 234579 [details] [review] installer: Add get_post_installable_drivers() Add method to fetch all post-installable device drivers.
Created attachment 234580 [details] [review] installer: Generalize setup_pre_install_driver*() Generalize setup_pre_install_driver*() so they are no longer specific to pre-installer drivers/scripts but rather use passed delegate to add files to either of the unattended files list. Naturally they are also renamed appropriately.
Created attachment 234581 [details] [review] configure: Require libosinfo >= 0.2.4
Created attachment 234582 [details] [review] installer: Ensure compatability between driver and scripts For each driver and script, make use of new libosinfo API to ensure: 1. Either the driver is signed or script can handle unsigned drivers. 2. Driver is of the same format as script expects.
Created attachment 234583 [details] [review] installer: Setup post-installation drivers
(In reply to comment #12) > Created an attachment (id=234581) [details] [review] > configure: Require libosinfo >= 0.2.4 This actually means the following changes require my yet-to-be-reviewed patches on libosinfo list.
Review of attachment 234575 [details] [review]: The commit log should indicate why we need to do this (cleaner, fixes a bug, needed by the subsequent patches, ...)
Review of attachment 234582 [details] [review]: s/compatability/compatibility/
Created attachment 235662 [details] [review] installer: Get rid of get_unattended_disk_info() method Keeping get_unattended_disk_info() separate from get_unattended_disk_config wasn't really helping readability much already and more so with the changes to follow.
Created attachment 235663 [details] [review] installer: Set bus on unattended disk config Don't exactly remember anymore if this is strictly needed but I don't think it'll do any harm to be explicit about the bus.
Created attachment 235664 [details] [review] installer: Update progress on error to setup driver
Created attachment 235665 [details] [review] installer: Add a secondary unattended disk In the following patches, we are going to add support for post-installation drivers and the first one to come from libosinfo is going to take 13M more. Its time to add a secondary disk in ISO format that we create on demand at runtime.
Created attachment 235666 [details] [review] installer: Add get_post_installer_scripts() Add method to fetch scripts that can install drivers at the end of OS installation.
Created attachment 235667 [details] [review] installer: Add get_post_installable_drivers() Add method to fetch all post-installable device drivers.
Created attachment 235668 [details] [review] installer: Generalize setup_pre_install_driver*() Generalize setup_pre_install_driver*() so they are no longer specific to pre-installer drivers/scripts but rather use passed delegate to add files to either of the unattended files list. Naturally they are also renamed appropriately.
Created attachment 235669 [details] [review] configure: Require libosinfo >= 0.2.4
Created attachment 235670 [details] [review] installer: Ensure compatibility between drivers and scripts For each driver and script, make use of new libosinfo API to ensure: 1. Either the driver is signed, script can handle unsigned drivers or driver signing checks can be disabled. 2. Driver is of the same format as script expects.
Created attachment 235671 [details] [review] installer: Disable driver signing checks Although this raises some security concerns as doing this for at least Windows XP mean disabling this permanently. i-e After installation users can shoot themselves in the feet by installing potentially dangerous drivers. However, this is the only possible way to get virtio and QXL device drivers installed out of the box. Perhaps this should be configurable through build-time configure option and/or a dconf setting?
Created attachment 235672 [details] [review] installer: Setup post-installation drivers
This version 2 of patches is mostly rebase/rework on top of the version 2 of related libosinfo patches + addressing the two comments from Christophe.
These are the required libosinfo patches: https://www.redhat.com/archives/libosinfo/2013-February/msg00026.html
Created attachment 236262 [details] [review] installer: Add a secondary unattended disk v3: Actually remove the disk from config after installation.
Created attachment 236263 [details] [review] installer: Add get_post_installer_scripts() v3: Update according to latest libosinfo API proposal.
Created attachment 236264 [details] [review] installer: Add get_post_installable_drivers() v3: Update according to latest libosinfo API proposal.
Created attachment 236265 [details] [review] installer: Ensure compatibility between drivers and scripts v3: Update according to latest libosinfo API proposal
Created attachment 236266 [details] [review] installer: Setup post-installation drivers v3: Update according to latest libosinfo API proposal
Comment on attachment 236265 [details] [review] installer: Ensure compatibility between drivers and scripts NM this one for now
Comment on attachment 235669 [details] [review] configure: Require libosinfo >= 0.2.4 this one too
Created attachment 236622 [details] [review] installer: Setup post-installation drivers Rebased after removing two patches under it.
No clue which patches are current, which are not, I guess everything before the 15th is obsolete, but even then this does not apply to git master in the order the patches are in this bug.
Review of attachment 236264 [details] [review]: ::: src/unattended-installer.vala @@ +703,3 @@ + + private GLib.List<DeviceDriver> get_post_installable_drivers () { + return get_drivers ((driver) => { return !driver.get_pre_installable (); }); The asymmetry with [PATCH] installer: Add get_post_installer_scripts() which uses "return script.get_can_post_install_dri.." is a bit unexpected
Review of attachment 236263 [details] [review]: ::: src/unattended-installer.vala @@ +724,3 @@ + return get_scripts ((script) => { return script.get_can_post_install_drivers (); }); + } + libosinfo associating drivers with a specific script combined with OS installers needing multiple install scripts is awkward imo, it would be much nicer if there was a higher level object combining all the install scripts together and which exposes the drivers.
(In reply to comment #39) > No clue which patches are current, which are not, I guess everything before the > 15th is obsolete, but even then this does not apply to git master in the order > the patches are in this bug. I have taken care of marking the obsolete patches as such. So all patches here needs to be reviewed.
(In reply to comment #42) > (In reply to comment #39) > > No clue which patches are current, which are not, I guess everything before the > > 15th is obsolete, but even then this does not apply to git master in the order > > the patches are in this bug. > > I have taken care of marking the obsolete patches as such. So all patches here > needs to be reviewed. http://bugzilla-attachments.gnome.org/attachment.cgi?id=235671 for one seems to be using non existing libosinfo API, some patches predate your v3 posting, are you 100% positive these 9 patches are needed?
(In reply to comment #43) > (In reply to comment #42) > > (In reply to comment #39) > > > No clue which patches are current, which are not, I guess everything before the > > > 15th is obsolete, but even then this does not apply to git master in the order > > > the patches are in this bug. > > > > I have taken care of marking the obsolete patches as such. So all patches here > > needs to be reviewed. > > http://bugzilla-attachments.gnome.org/attachment.cgi?id=235671 for one seems to > be using non existing libosinfo API, some patches predate your v3 posting, are > you 100% positive these 9 patches are needed? Ah, I missed that one but now its obsoleted. All others seem right.
Review of attachment 236262 [details] [review]: ::: src/unattended-installer.vala @@ +58,2 @@ public File? disk_file; + public File? secondary_disk_file; "drivers_disk_file" would be more descriptive (let's say preinst drivers are a bit special, and that this naming won't be too confusing) @@ +173,3 @@ + + debug ("Creating secondary disk image '%s'...", secondary_disk_path); + string[] argv = { "mkisofs", "-graft-points", "-J", "-rock", "-o", secondary_disk_path }; "All occurrences of `\' and `=' characters must be escaped with `\' if -graft-points has been specified." @@ +512,3 @@ disk.set_source (disk_file.get_path ()); + if (disk_file == this.disk_file) { 2 distinct functions get_unattended_disk_config and get_drivers_disk_config please, this one is just weird
(In reply to comment #44) > > Ah, I missed that one but now its obsoleted. All others seem right. still does not apply cleanly here
Review of attachment 235662 [details] [review]: (-ènxdferfvg
Review of attachment 235663 [details] [review]: Failed to publish review: Undefined subroutine &extensions::splinter::lib::WSSplinter::ThrowCodeError called at /var/www/bugzilla.gnome.org/extensions/splinter/lib/WSSplinter.pm line 88.
Review of attachment 235664 [details] [review]: I'd be more verbose in the long commit log, something like 'make sure driver installation progress always reaches 100% even if an error occurs during the installation"
Review of attachment 235668 [details] [review]: ::: src/unattended-installer.vala @@ +635,3 @@ + + private async void setup_drivers (ActivityProgress progress, Cancellable? cancellable = null) { + progress.info = _("Downloading device drivers..."); This was not present in the old code.. @@ +659,3 @@ + } + } else + progress.progress = 1.0; ... and this seems new as well, as both of these seem to be cosmetic improvements to the progress display, they may be better in a separate patch.
Review of attachment 236622 [details] [review]: ok let's anticipate splinter's requiring a comment when all I want to do is change the status. BONJOUR!!
Review of attachment 236262 [details] [review]: ::: src/unattended-installer.vala @@ +58,2 @@ public File? disk_file; + public File? secondary_disk_file; not sure this will always be the case, was even thinking that it might be better to put user avatar there already. You never know if someone got a 1.44 MB avatar. :) @@ +173,3 @@ + + debug ("Creating secondary disk image '%s'...", secondary_disk_path); + string[] argv = { "mkisofs", "-graft-points", "-J", "-rock", "-o", secondary_disk_path }; Hmm.. is that just for the commandline or filenames actually have the escape sequence then on the ISO? If its latter, this won't be easy to handle correctly, at least for the scripts as they have expected filenames.
Review of attachment 235668 [details] [review]: ::: src/unattended-installer.vala @@ +635,3 @@ + + private async void setup_drivers (ActivityProgress progress, Cancellable? cancellable = null) { + progress.info = _("Downloading device drivers..."); It was. It was just in a different function. @@ -641,1 @@ - progress.info = _("Downloading device drivers..."); Here it is.
Review of attachment 236262 [details] [review]: ::: src/unattended-installer.vala @@ +58,2 @@ public File? disk_file; + public File? secondary_disk_file; ok, worth adding a comment indicating what goes in this secondary file then (and a similar one for the unattended disk) @@ +173,3 @@ + + debug ("Creating secondary disk image '%s'...", secondary_disk_path); + string[] argv = { "mkisofs", "-graft-points", "-J", "-rock", "-o", secondary_disk_path }; My understanding is that it's just for mkisofs use, this way it can split on the only = that is not escaped, and then unescape the part before and the part after the "=" and use these unescaped names as filenames
Created attachment 236823 [details] [review] installer: Update progress on error to setup driver Ensure driver installation progress always reaches 100% even if an error occurs during the installation. Also moved one hunk from 'Generalize setup_pre_install_driver*()' patch to this one as it fits here better.
Created attachment 236824 [details] [review] installer: Add a secondary unattended disk Addressed review comments.
Created attachment 236825 [details] [review] installer: Generalize setup_pre_install_driver*() Rebased over other changes and some review comments addressed.
Created attachment 236827 [details] [review] installer: Setup post-installation drivers Rebased.
Review of attachment 236823 [details] [review]: ack
Review of attachment 236824 [details] [review]: ack
Review of attachment 236825 [details] [review]: ack
Review of attachment 236827 [details] [review]: ack
Attachment 235662 [details] pushed as ef43977 - installer: Get rid of get_unattended_disk_info() method Attachment 235663 [details] pushed as b381592 - installer: Set bus on unattended disk config Attachment 236263 [details] pushed as 4014fab - installer: Add get_post_installer_scripts() Attachment 236264 [details] pushed as ad3a050 - installer: Add get_post_installable_drivers() Attachment 236823 [details] pushed as 443af23 - installer: Update progress on error to setup driver Attachment 236824 [details] pushed as 4df788c - installer: Add a secondary unattended disk Attachment 236825 [details] pushed as dfc2c37 - installer: Generalize setup_pre_install_driver*() Attachment 236827 [details] pushed as d599f3a - installer: Setup post-installation drivers
These changes combined with latest libosinfo automatically add QXL drivers for Windows XP. I'll open a separate bug for windows 7.
Re-opening since it turns out that I got QXL working in neither windows 7 nor XP. It does get installed though along with vdagent and virtio drivers do work, which do work. Will attach patches to fix this but it'll need libosinfo patching as well.
Created attachment 238951 [details] [review] configure: Require libosinfo >= 0.2.6
Created attachment 238952 [details] [review] installer: Disable driver signing In the next release (0.2.6) of libosinfo, driver signing checks in scripts will have to be explicitly disabled if we want to use unsigned drivers. That is something we really do want to do since QXL drivers for windows are not signed and they are essential for an impressive user experience.
Created attachment 238953 [details] [review] installer: Ensure compatibility between drivers and scripts For each driver and script, make use of new libosinfo API to ensure that either the driver is signed, script can handle unsigned drivers or there is a script in the 'desktop' profile that provide a way to disable driver signing checks. With this and the previous ('installer: Disable driver signing') change, QXL driver will now actually be functional in Windows XP and Windows 7.
Created attachment 238954 [details] [review] installer: Filter drivers by architecture first Just like media, the usual schenerio would be for an OS to provide multiple variants of the same driver, one for each architecture. So its more optimal to first filter devices by compatible architecture and then filter the resulting drivers list by other compatibility criterias.
Review of attachment 238952 [details] [review]: ::: src/unattended-installer.vala @@ +258,3 @@ } + + config.set_driver_signing (false); Let's not hardcode this as libosinfo provides the api to know if we actually need to disable signing or not.
Created attachment 238983 [details] [review] installer: Ensure compatibility between drivers and scripts For each driver and script, make use of new libosinfo API to ensure that either the driver is signed or script can handle unsigned drivers.
Created attachment 238984 [details] [review] installer: Disable driver signing for unsigned drivers In the next release (0.2.6) of libosinfo, driver signing checks in scripts will have to be explicitly disabled if we want to use unsigned drivers. We actually shouldn't be doing this since this means disabling a security feature of Windows *permanently* but unfortunately QXL drivers for windows are not signed and they are essential for an impressive user experience. To make things slightly better, we only do this if there is an unsigned driver to be installed. With this change QXL driver will now actually be functional in Windows XP and Windows 7.
Review of attachment 238983 [details] [review]: ::: src/unattended-installer.vala @@ +754,3 @@ + return false; + } + } This foreach loop is really something that should be done by a helper func provided by libosinfo.
Review of attachment 238983 [details] [review]: ::: src/unattended-installer.vala @@ +754,3 @@ + return false; + } + } And it's not very nice that we compute this once for each driver while this value will always be the same for a call to get_installable_drivers
Review of attachment 238984 [details] [review]: ::: src/unattended-installer.vala @@ +771,3 @@ + debug ("Script '%s' allows disabling of driver signature checks.", script.id); + driver_signing = false; + return true; One more loop that is needlessly ran several times. Also, I don't think libosinfo documents the fact that if one script allows to disable signature checks, then it will disable these checks for any driver installable by any install script for the same OS (which is what you are relying on here).
Review of attachment 238954 [details] [review]: NACK on the 'more optimal' part, if 'test_func()' just returns 'FALSE', I doubt this will be a performance improvement (which 'optimal' makes me think of). Use 'more logical', 'makes more sense', if you want. s/schenerio/scenario And to be honest, unconvinced this is a very useful improvement overall, the way the code is written before this patch seems more logical to me, we prune drivers we don't want without even looking at the arch as soon as possible rather than keeping the unwanted drivers as long as possible, and dropping them at the last minute. If the driver hash table were to be persistent across get_drivers() calls for some reason, then this would be a different story.
Review of attachment 238983 [details] [review]: ::: src/unattended-installer.vala @@ +754,3 @@ + return false; + } + } Good point. I'll fix.
Review of attachment 238984 [details] [review]: ::: src/unattended-installer.vala @@ +771,3 @@ + debug ("Script '%s' allows disabling of driver signature checks.", script.id); + driver_signing = false; + return true; * Yeah, I'll move this out of loop as well. * Firstly we only take "desktop" profile scripts. 2ndly, check the docs for OSINFO_DEVICE_DRIVER_SIGNING_REQ_STRICT: "Script must only be given signed device drivers. Some scripts will allow overriding this requirement through osinfo_install_config_set_driver_signing function. You can query if a script supports this by checking if OSINFO_INSTALL_CONFIG_PROP_DRIVER_SIGNING configuration parameter is used by the script in question (or other scripts in the same profile)." Trouble is that we can't guarantee the same script that uses/installs drivers will be able to disable driver signing for us (e.g in XP's case I was only able to disable through .sif file and drivers are not installed by .sif). Hence such a non-unified API.
Review of attachment 238954 [details] [review]: I don't agree about "your last minute" part, we are doing a check just before adding it to the list of drivers we want, nothing very "last minute" about that. Perhaps you'll feel better if both checks remain in the same loop? I think there is no sense in first checking if a driver is something we want or not and if we do, we check if the driver was relevant in the first place. We should simply do it the other way around: First check if driver is relevant and then do other policy-related checks.
(switching to a saner way of answering comments) (In reply to comment #79) > * Firstly we only take "desktop" profile scripts. 2ndly, check the docs for > OSINFO_DEVICE_DRIVER_SIGNING_REQ_STRICT: "Script must only be given signed > device drivers. Some scripts will allow overriding this requirement through > osinfo_install_config_set_driver_signing function. You can query if a script > supports this by checking if OSINFO_INSTALL_CONFIG_PROP_DRIVER_SIGNING > configuration parameter is used by the script in question (or other scripts in > the same profile)." > > Trouble is that we can't guarantee the same script that uses/installs drivers > will be able to disable driver signing for us (e.g in XP's case I was only able > to disable through .sif file and drivers are not installed by .sif). Hence such > a non-unified API. I know about this requirement, and I've read the doc you quote. I still think how to disable signing is not very obvious. All in all, this libosinfo API seems awkward to use, something slightly higher level seems like it would be a better fit.
(In reply to comment #80) > Review of attachment 238954 [details] [review]: > > I don't agree about "your last minute" part, we are doing a check just before > adding it to the list of drivers we want, nothing very "last minute" about > that. s/last minute/last/ if you prefer, don't focus too much on the words ;) instead of driver check first, arch check last, you do driver check last. > I think there is no sense in first checking if a driver is something we want or > not and if we do, we check if the driver was relevant in the first place. We > should simply do it the other way around: First check if driver is relevant and > then do other policy-related checks. If we could get some false positive by doing the check in the 'wrong' order, then the order would matter, in this case, I'm unconvinced changing the code is worth it. This is a case where either way does not really matter imo.
(In reply to comment #81) > (switching to a saner way of answering comments) > > > (In reply to comment #79) > > * Firstly we only take "desktop" profile scripts. 2ndly, check the docs for > > OSINFO_DEVICE_DRIVER_SIGNING_REQ_STRICT: "Script must only be given signed > > device drivers. Some scripts will allow overriding this requirement through > > osinfo_install_config_set_driver_signing function. You can query if a script > > supports this by checking if OSINFO_INSTALL_CONFIG_PROP_DRIVER_SIGNING > > configuration parameter is used by the script in question (or other scripts in > > the same profile)." > > > > Trouble is that we can't guarantee the same script that uses/installs drivers > > will be able to disable driver signing for us (e.g in XP's case I was only able > > to disable through .sif file and drivers are not installed by .sif). Hence such > > a non-unified API. > > I know about this requirement, and I've read the doc you quote. I still think > how to disable signing is not very obvious. All in all, this libosinfo API > seems awkward to use, something slightly higher level seems like it would be a > better fit. As I said in the libosinfo patch review, I agree about that. Although I don't think I have time to add that higher level utility API today/before release.
(In reply to comment #82) > (In reply to comment #80) > > Review of attachment 238954 [details] [review] [details]: > > > > I don't agree about "your last minute" part, we are doing a check just before > > adding it to the list of drivers we want, nothing very "last minute" about > > that. > > s/last minute/last/ if you prefer, don't focus too much on the words ;) instead > of driver check first, arch check last, you do driver check last. > > > I think there is no sense in first checking if a driver is something we want or > > not and if we do, we check if the driver was relevant in the first place. We > > should simply do it the other way around: First check if driver is relevant and > > then do other policy-related checks. > > If we could get some false positive by doing the check in the 'wrong' order, > then the order would matter, in this case, I'm unconvinced changing the code is > worth it. This is a case where either way does not really matter imo. If we got false positives, this won't be about being optimal. There will always be at least 2 driver entries for i686 and x86_64 for each driver. That would mean that we always at least do 1 redundant check for each driver. That not a big improvement in performance at all but now that I've done the work, I don't see why you wan to block this small improvement.
(In reply to comment #84) > That not a > big improvement in performance at all but now that I've done the work, I don't > see why you wan to block this small improvement. Ok, so once again your are pushing this for the wrong reasons, this is *not* a performance improvement to me, just some code movement that *could* make some non-critical code a bit faster. As I said, 'This is a case where either way does not really matter imo.', and no code change is generally better than unneeded code change ;)
(In reply to comment #83) > As I said in the libosinfo patch review, I agree about that. Although I don't > think I have time to add that higher level utility API today/before release. Too bad this means this suboptimal API gets set in stone while we already know it will need to be improved :(
(In reply to comment #86) > (In reply to comment #83) > > As I said in the libosinfo patch review, I agree about that. Although I don't > > think I have time to add that higher level utility API today/before release. > > Too bad this means this suboptimal API gets set in stone while we already know > it will need to be improved :( No, I didn't say that we'll change this API but rather add new one on top that will be more high-level/convenient.
(In reply to comment #85) > (In reply to comment #84) > > That not a > > big improvement in performance at all but now that I've done the work, I don't > > see why you wan to block this small improvement. > > Ok, so once again your are pushing this for the wrong reasons, this is *not* a > performance improvement to me, just some code movement that *could* make some > non-critical code a bit faster. That statement is just a play of words. Making any code faster *is* performance improvement, not matter how tiny. > As I said, 'This is a case where either way > does not really matter imo.', and You are insisting on that opinion of yours even though I already showed how this is not the case. Hence I'll ignore all future comments from you on this patch. I got better things to do then justifying simple improvement patches that should just go in w/o reivew even. In future, I'll push such patches directly.
(In reply to comment #88) > That statement is just a play of words. Making any code faster *is* performance > improvement, not matter how tiny. I've given one example where your change would not make code faster, a noop 'driver_func'. > I got better things to do then justifying simple improvement patches > that should just go in w/o reivew even. This is not about 'justifying', I'm just asking you not to push code for a misleading reason.
(In reply to comment #88) > In future, I'll push such patches directly. NACK, this is not something you unilaterally decide in a bug report other members of the team may not be paying attention to.
(In reply to comment #87) > No, I didn't say that we'll change this API but rather add new one on top that > will be more high-level/convenient. Yup, and I suspect we'll regret having added this low-level API in the first place...
Created attachment 239163 [details] [review] installer: Ensure compatibility between drivers and scripts v2: Avoid redundant loops.
Created attachment 239164 [details] [review] installer: Disable driver signing for unsigned drivers v2: Avoid redundant loops.
Review of attachment 238954 [details] [review]: I'd rather that you ask nicely to get such patches in rather than repeating the same invalid arguments several times, and then getting angry.
Review of attachment 239163 [details] [review]: Failed to publish review: Undefined subroutine &extensions::splinter::lib::WSSplinter::ThrowCodeError called at /var/www/bugzilla.gnome.org/extensions/splinter/lib/WSSplinter.pm line 88.
Review of attachment 239164 [details] [review]: ::: src/unattended-installer.vala @@ +124,3 @@ + if (script.has_config_param_name (INSTALL_CONFIG_PROP_DRIVER_SIGNING)) { + debug ("Script '%s' allows disabling of driver signature checks.", script.id); + driver_signing_mutable = true; get_installable_drivers() has a loop iterating over the install scripts, why is it not done there?
Review of attachment 239163 [details] [review]: Code looks much better this way btw!
Review of attachment 239164 [details] [review]: ::: src/unattended-installer.vala @@ +124,3 @@ + if (script.has_config_param_name (INSTALL_CONFIG_PROP_DRIVER_SIGNING)) { + debug ("Script '%s' allows disabling of driver signature checks.", script.id); + driver_signing_mutable = true; This is also a loop already iterating over all scripts so we can do it here or there. I would move that code in this loop as well if it made sense but it doesn't. Moreover that loop: * is exited as soon as we find a strict requirement. * is run twice for each type of script (pre/post installable). So that reminds me that I should make that loop (in get_installable_drivers()) only iterate over the scripts it should iterate on instead..
Created attachment 239215 [details] [review] installer: Ensure compatibility between drivers and scripts In this version, we now only loop over relavent scripts rather than all of them.
Review of attachment 239164 [details] [review]: ::: src/unattended-installer.vala @@ +124,3 @@ + if (script.has_config_param_name (INSTALL_CONFIG_PROP_DRIVER_SIGNING)) { + debug ("Script '%s' allows disabling of driver signature checks.", script.id); + driver_signing_mutable = true; Why doesn't it make sense to have that code in that loop? The loop you have changed here does nothing related to signing, while the one in get_installed_drivers is already doing signing related work. Moreover, the only part of the code which needs the new 'driver_signing_mutable' attribute is the get_installed_drivers() functions, so if the loop was there, this attribute could be a local variable instead.
Review of attachment 239215 [details] [review]: An additional patch on top of the already ACK'ed one would have saved some review time... ;)
Review of attachment 239164 [details] [review]: ::: src/unattended-installer.vala @@ +124,3 @@ + if (script.has_config_param_name (INSTALL_CONFIG_PROP_DRIVER_SIGNING)) { + debug ("Script '%s' allows disabling of driver signature checks.", script.id); + driver_signing_mutable = true; If that loop had remained the same, the only reason to not move this code there would be efficiency (not have to loop though the whole list both times that function is called, so not really a good one. However, now that loop only iterates over the relevant scripts (pre- or post- installation scripts) with the new patch that you ACK'ed. OTOH this code needs to loop through all scripts.
Review of attachment 239164 [details] [review]: ::: src/unattended-installer.vala @@ +124,3 @@ + if (script.has_config_param_name (INSTALL_CONFIG_PROP_DRIVER_SIGNING)) { + debug ("Script '%s' allows disabling of driver signature checks.", script.id); + driver_signing_mutable = true; Then let's just have a second loop through all the scripts in get_installed_drivers() and have most of the signing code gathered in that function.
Created attachment 239257 [details] [review] installer: Disable driver signing for unsigned drivers Moved the loop as per comment#103.
Review of attachment 239257 [details] [review]: Thanks! ::: src/unattended-installer.vala @@ +773,3 @@ return false; if (!driver.get_signed () && signing_required) { we could probably have a if (!driver_signing) return true; before this test, but that's not very important.
Attachment 238951 [details] pushed as 1325d4b - configure: Require libosinfo >= 0.2.6 Attachment 238954 [details] pushed as 864a474 - installer: Filter drivers by architecture first Attachment 239215 [details] pushed as c2d456c - installer: Ensure compatibility between drivers and scripts Attachment 239257 [details] pushed as f8e88e8 - installer: Disable driver signing for unsigned drivers