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

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?
>  
>    (* 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:

 - It should use windows_systemroot, instead of "/Windows"

 - It doesn't handle case sensitive path stuff

Do we still need to check for the registry key?  (I have no idea)
> +    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 ...

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).

Rich.
> +    oem_inf
> +
>  (* Copy the matching drivers to the driverdir; return true if any have
>   * been copied.
>   *)
> -- 
> 2.6.2
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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@redhat.com
> > https://www.redhat.com/mailman/listinfo/libguestfs
>

On Tue, Apr 05, 2016 at 02:24:33PM +0200, Cedric Bosdonnat
wrote:
> 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?
That would seem to be more accurate.

On the other points, I'd like to see what Roman suggests.  Maybe
we can just make up a random name instead of using oemNN.inf?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v