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

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 at virtuozzo.com>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>
> ---
>  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/b28cd1dcfeb40e7002e8d0b0ce9dcc4ce86beb6c

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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 at virtuozzo.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at
virtuozzo.com>
>> ---
>>  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/b28cd1dcfeb40e7002e8d0b0ce9dcc4ce86beb6c
> 
> 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
> 
> Rich.
>