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

[Richard W.M. Jones]

On Thu, Mar 09, 2023 at 04:38:44PM +0200, Andrey Drobyshev wrote:
> 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

Seems about the same as the original patch really.

But yes, your choice here.  You could leave it as in the original version.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit