[Libguestfs] [PATCH 1/7] v2v: check next free oem%d.inf in /Windows/Inf

[Cedric Bosdonnat]

On Tue, 2016-04-05 at 13:04 +0100, Richard W.M. Jones wrote:
> On Tue, Apr 05, 2016 at 01:47:27PM +0200, Cédric Bosdonnat wrote:
> > +  let oem_inf = set_free_oem_inf g root scsi_adapter_guid
> > "viostor.inf" driverdir in
> 
> Seems better if it was called *get_next*_free_oem_inf?

Yes, sounds better.

> >    (* There should be a key
> >     *   HKLM\SYSTEM\ControlSet001\Control\Class\<scsi_adapter_guid>
> > @@ -398,6 +378,28 @@ and add_viostor_to_driver_database g root arch
> > current_cs =
> >        
> >  @=hex(ffff0012):6f,00,65,00,6d,00,31,00,2e,00,69,00,6e,00,66,00,00
> > ,00
> >  *)
> >  
> > +(* There should be a key
> > + *   HKLM\SYSTEM\DriverDatabase\DeviceIds\<guid>
> > + * We want to add:
> > + *   "oem1.inf"=hex(0):
> > + * but if we find "oem1.inf" we'll add "oem2.inf" (etc).
> > + *)
> > +and set_free_oem_inf g root guid driver_inf driverdir =
> > +  let path = [ "DriverDatabase"; "DeviceIds"; guid ] in
> > +  match Windows.get_node g root path with
> > +  | None ->
> > +     error (f_"cannot find
> > HKLM\\SYSTEM\\DriverDatabase\\DeviceIds\\%s in the guest registry")
> > guid
> > +  | Some node ->
> > +    let rec loop i =
> > +      let oem_inf = sprintf "oem%d.inf" i in
> > +      if not (g#exists ("/Windows/Inf/" ^ oem_inf)) then oem_inf
> > else loop (i+1)
> > +    in
> 
> This bit doesn't match what is described in the comment.  It's also
> incorrect for a few reasons:

May be I should add to the comment that it searches for the next
available oem%d.inf in the Windows\Inf folder?

>  - It should use windows_systemroot, instead of "/Windows"
> 
>  - It doesn't handle case sensitive path stuff

Indeed, I'll need to fix those

> Do we still need to check for the registry key?  (I have no idea)

As we add the key just a few lines below, we need to check for it


> > +    let oem_inf = loop 1 in
> > +    (* Create the key. *)
> > +    g#hivex_node_set_value node oem_inf (* REG_NONE *) 0_L "";
> > +    g#cp (driverdir // driver_inf) ("/Windows/Inf/" ^ oem_inf);
> 
> And this line seems like a bit of a hack.  We have a place where
> drivers are copied into the driverdir.  I think it would be better if
> we returned oem_inf from add_viostor_to_registry. 
>  But ...

It's not exactly in driverdir, it's in a subfolder of it. The problem
is that it seems that Windows gets the next available oem%d.inf from
the files in /Windows/Inf... if we aren't copying them manually while
we reserve it, we end up with conflicts when we have more than one
driver to add.

I don't see why you say you'ld see oem_inf returned from
 add_viostor_to_registry: we don't need it outside that function,
right?

> Do we actually need to do this copy at all?  The Red Hat drivers
> don't
> require this, in order to boot (note that with the Red Hat drivers we
> only half install them, they are properly installed when Windows
> boots).

The virtio block driver from SUSE VMDP has a dependency on the balloon
driver. That's why we need to half install these two before the first
boot.

The situation is cleaned up at first boot and the other drivers are
installed at that time too.

--
Cedric

> Rich.
> 
> > +    oem_inf
> > +
> >  (* Copy the matching drivers to the driverdir; return true if any
> > have
> >   * been copied.
> >   *)
> > -- 
> > 2.6.2
> > 
> > _______________________________________________
> > Libguestfs mailing list
> > Libguestfs at redhat.com
> > https://www.redhat.com/mailman/listinfo/libguestfs
>