Created attachment 244457 [details] [review]
Revert "installer: Clean-up supported devices related code"

This reverts commit e69add66a4c7f07c133e543eaa9e5d11827525a7.

Using a single list of devices for the devices supported by
the OS and the ones supported through the addition of a device
driver causes problems with non-express installs as the
drivers are downloaded and added to the list of supported
devices before the user gets to make a decision between
express/non-express.

This causes problems with non-express Windows installations
as the supported_devices list will contain a virtio disk
controller, but when we disable express install, we can
no longer use a virtio controller for the installation, but
the VM will still try to use a virtio controller, which
makes it impossible to do the Windows install.

This revert goes back to using 2 separate list for devices
natively supported by the OS, and devices supported
through the addition of device drivers. Given that
drivers are setup during the 'prepare' step, and given that
the user can freely enable/disable express installation (ie
use of these drivers) after this 'prepare' step, we have to store
which devices can be used natively, and which one need a driver.
So it's more convenient to go back to where we were before this
'cleanup'.

Created attachment 244458 [details] [review]
Only use virtio in express install and when we have drivers

Currently, as long as we successfully located the virtio drivers,
supports_virtio_disk will return TRUE. This is not correct when
express install is disabled as the virtio drivers won't be
automatically installed, and the installer will not be able
to make use of them.
Only take virtio drivers into account in supports_virtio_disk when
we are doing an express installation.

Review of attachment 244457 [details] [review]:

I don't think there is any need to revert to old code. We don't exactly need to differentiate between natively supported and other drivers but rather devices that are available and ones that are not. I'd rather we keep UnattendedInstaller should keep a separate/private list of additional devices and add/remove them to/from 'InstallerMedia.supported_devices' when express installation is toggled on/off.

Review of attachment 244457 [details] [review]:

It's much less work this way, and also faster at runtime.

Review of attachment 244457 [details] [review]:

Or we could also do something like:

In InstallerMedia;

public virtual Osinfo.DeviceList supported_devices { get { os.get_devices (); } }

In UnattendedInstaller:

private Osinfo.DeviceList additional_devices;
public override Osinfo.DeviceList supported_devices { get {
   if (express_install)
      return base.supported_devices + addition_devices;
   else 
      return base.supported_devices;

} }

BTW the latter part of "This revert goes back to using 2 separate list for devices
natively supported by the OS, and devices supported
through the addition of device drivers" in the commit log is not quite correct. We don't maintain any list after this patch and get the natively supported devices list from the OS at runtime.

::: src/unattended-installer.vala
@@ +692,3 @@
+                                                               cancellable);
+
+            foreach (var driver in setup_drivers) {

IIRC the main point of the 'cleanup' patch you are reverting was to get rid of this hack looking code here.

Review of attachment 244457 [details] [review]:

What you suggest is still less efficient that the proposed solution.

::: src/unattended-installer.vala
@@ +692,3 @@
+                                                               cancellable);
+
+            foreach (var driver in setup_drivers) {

It does not really get rid of it, it does the same thing using generic libosinfo functions, the loop looks like it can be replaced by 

foreach (var driver in setup_drivers) {
  if (find_device_by_prop(driver.get_devices(), DEVICE_PROP_NAME, "virtio-block) != null) {
    has_viostor_drivers = true;
    break;
  }
}

Review of attachment 244457 [details] [review]:

"What you suggest is still less efficient that the proposed solution."

Yes but not much at all and only when unattended installer is used.

::: src/unattended-installer.vala
@@ +692,3 @@
+                                                               cancellable);
+
+            foreach (var driver in setup_drivers) {

Looping over the setup devices to set the boolean variable is what seems hackish to me.

Review of attachment 244457 [details] [review]:

Sorry, but you seemed to care a lot about minor performance improvements in bug #691546, now is my turn to decide we have to be efficient here.

::: src/unattended-installer.vala
@@ +692,3 @@
+                                                               cancellable);
+
+            foreach (var driver in setup_drivers) {

Looping every time over a list that does not change to compute  a boolean value could be given various not nice names as well.

Review of attachment 244457 [details] [review]:

We are not playing the game of "Its my turn to care about optimization and you must accept what I say when its my turn". You are making pretty intrusive changes for fixing a bug that doesn't really require it so it needs good justification.

With your patch, we still do:

public virtual bool supports_virtio_disk {
         get {
             return (find_device_by_prop (supported_devices, DEVICE_PROP_NAME, "virtio-block") != null);
         }
     }

Which is not way too efficient either. What I suggested extends on this code and makes the code cleaner by using a similar logic in the subclass for this property.

Review of attachment 244457 [details] [review]:

Think about the potential consequences _before_ insisting to get a patch in then if you want to have some credibility when using otherwise good arguments.

Since it's a revert of a cleanup, the 'invasive change' is going back to a known working state, so it's much less worrying than if it was some new code written from scratch

Review of attachment 244457 [details] [review]:

So this is now going to be a discussion about making me feel bad about what I did in the past and use that as arguments? Also my credibility has nothing to do with this bug. Please stop insulting to get your patch in.

Review of attachment 244457 [details] [review]:

I'm not insulting you. I'm just telling you why I'll ignore your comments about the performance gains being irrelevant.

Review of attachment 244457 [details] [review]:

I guess I'll let you propose a patch implementing what you suggested.

Review of attachment 244458 [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.

Created attachment 244780 [details] [review]
Disabling express install disables additional devices

Keep additional devices that are installed as part of unattended
installation, in a separate list to ensure that we don't assume their
existence when express installation is disabled by user.

Created attachment 244781 [details] [review]
Disabling express install disables additional devices

We were assuming the existance of additional devices even when user was
choosing to disable express installation while these devices are only
made available through express installation. This effectively meant
broken non-express installation for Windows XP and 7.

Keep additional devices in a separate list to ensure that we don't assume
their existence when express installation is disabled by user.

Created attachment 244889 [details] [review]
Disabling express install disables additional devices

Rebased.

Review of attachment 244889 [details] [review]:

This is even more inefficient than I thought :(

Attachment 244889 [details] pushed as 82199fc - Disabling express install disables additional devices

Review of attachment 244889 [details] [review]:

::: src/installer-media.vala
@@ -123,3 @@
 
-    private void init_supported_devices () {
-        if (os != null) {

You lost this check when moving this code to supported_devices, this causes a crash when starting a f19 live cd

Created attachment 244929 [details] [review]
installer: Don't assume OS to be always known

This fixes regression intruduce by commit 82199fc (Disabling express
install disables additional devices).

Attachment 244929 [details] pushed as 9061bd3 - installer: Don't assume OS to be always known