Re: [Libguestfs] [COMMON PATCH v2 4/4] inject_virtio_win: write the proper block controller PCI ID to Win registry

On 3/8/23 22:45, Richard W.M. Jones wrote: > On Tue, Mar 07, 2023 at 09:40:26PM +0200, Andrey Drobyshev wrote: >> In case when we are injecting virtio-scsi device driver into the guest >> (rather than the default virtio-blk), make sure we write the right PCI ID >> value into the Windows guest registry. This is essential for the guest >> to be bootable afterwards. >> >> Originally-by: Roman Kagan <rkagan(a)virtuozzo.com&gt; >> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; >> --- >> mlcustomize/inject_virtio_win.ml | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml >> index 345fe32..922c1ab 100644 >> --- a/mlcustomize/inject_virtio_win.ml >> +++ b/mlcustomize/inject_virtio_win.ml >> @@ -207,10 +207,16 @@ let rec inject_virtio_win_drivers ({ g } as t) reg = >> let target = sprintf "%s/system32/drivers/%s.sys" >> t.i_windows_systemroot driver_name in >> let target = g#case_sensitive_path target in >> + let installed_block_type, legacy_pciid, modern_pciid = ( >> + if driver_name = "vioscsi" then >> + Virtio_SCSI, vioscsi_legacy_pciid, vioscsi_modern_pciid >> + else >> + Virtio_blk, viostor_legacy_pciid, viostor_modern_pciid >> + ) in >> g#cp source target; >> - add_guestor_to_registry t reg driver_name viostor_legacy_pciid; >> - add_guestor_to_registry t reg driver_name viostor_modern_pciid; >> - Virtio_blk in >> + add_guestor_to_registry t reg driver_name legacy_pciid; >> + add_guestor_to_registry t reg driver_name modern_pciid; >> + installed_block_type in >> >> (* Can we install the virtio-net driver? *) >> let net : net_type = > > Could we make this look more like the code before it was reverted > here? > > https://github.com/libguestfs/virt-v2v/commit/b28cd1dcfeb40e7002e8d0b0ce9... > > See the lines starting: > | Some Virtio_SCSI, _, true -> > (* Block driver needs tweaks to allow booting; the rest is set up by PnP > ... > > So the change would look like: > > - | Some driver_name -> > + | Some "viostor" -> > (* Block driver needs tweaks to allow booting; > * the rest is set up by PnP manager. > *) > let source = driverdir // (driver_name ^ ".sys") in > let target = sprintf "%s/system32/drivers/%s.sys" > t.i_windows_systemroot driver_name in > let target = g#case_sensitive_path target in > g#cp source target; > add_guestor_to_registry t reg driver_name viostor_legacy_pciid; > add_guestor_to_registry t reg driver_name viostor_modern_pciid; > Virtio_blk in > > + | Some "vioscsi" -> > + [... new code for the virtio-scsi case ...] > > > The old code was a bit clearer don't you think? Even if there's a > little bit more duplication. IMO evaluating driver path and adding registry value separately for each and every case seems like too much duplication. If you think string comparison looks a bit clumsy here, how about nesting another level of matching, like so: > | Some driver_name -> > (* Block driver needs tweaks to allow booting; > * the rest is set up by PnP manager. > *) > let source = driverdir // (driver_name ^ ".sys") in > let target = sprintf "%s/system32/drivers/%s.sys" > t.i_windows_systemroot driver_name in > let target = g#case_sensitive_path target in > let installed_block_type, legacy_pciid, modern_pciid = > match driver_name with > | "vioscsi" -> Virtio_SCSI, vioscsi_legacy_pciid, vioscsi_modern_pciid > | _ -> Virtio_blk, viostor_legacy_pciid, viostor_modern_pciid > in > g#cp source target; > add_guestor_to_registry t reg driver_name legacy_pciid; > add_guestor_to_registry t reg driver_name modern_pciid; > installed_block_type in