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

2016-04-05 Thread 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\
> > @@ -398,6 +378,28 @@ and add_viostor_to_driver_database g root arch
> > current_cs =
> >
> >  @=hex(0012):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\
> > + * 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
> 

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

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

2016-04-05 Thread Cedric Bosdonnat
On Tue, 2016-04-05 at 15:13 +0300, Roman Kagan wrote:
> On Tue, Apr 05, 2016 at 01:47:27PM +0200, Cédric Bosdonnat wrote:
> > It seems that checking for oem%d.inf in the DeviceIds registry
> > entry
> > doesn't always list all oemXX.inf files. For example we may have
> > oem1.inf free in the registry key, but used in another one.
> 
> In my experiments this name doesn't have to bear any sense, so
> there's
> no point doing anything smart about it.

What I did was not a strictly scientific approach, rather trying to
mimic what seemed to happen ;)

> FWIW I've been working on an alternative apporoach (mostly about
> reducing the amount of regedits) and was about to submit the patches
> today.  I guess it'd accomodate SUSE driver suite just as well.

Cool, I'ld be happy to test your patches together with mines ;)

--
Cedric

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

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

2016-04-05 Thread Richard W.M. Jones
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\
> > > @@ -398,6 +378,28 @@ and add_viostor_to_driver_database g root arch
> > > current_cs =
> > >
> > >  @=hex(0012):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\
> > > + * 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

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs


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

2016-04-05 Thread Roman Kagan
On Tue, Apr 05, 2016 at 01:47:27PM +0200, Cédric Bosdonnat wrote:
> It seems that checking for oem%d.inf in the DeviceIds registry entry
> doesn't always list all oemXX.inf files. For example we may have
> oem1.inf free in the registry key, but used in another one.

In my experiments this name doesn't have to bear any sense, so there's
no point doing anything smart about it.

FWIW I've been working on an alternative apporoach (mostly about
reducing the amount of regedits) and was about to submit the patches
today.  I guess it'd accomodate SUSE driver suite just as well.

Roman.

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs


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

2016-04-05 Thread Richard W.M. Jones
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\
> @@ -398,6 +378,28 @@ and add_viostor_to_driver_database g root arch 
> current_cs =
> @=hex(0012):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\
> + * 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

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs


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

2016-04-05 Thread Cédric Bosdonnat
It seems that checking for oem%d.inf in the DeviceIds registry entry
doesn't always list all oemXX.inf files. For example we may have
oem1.inf free in the registry key, but used in another one.

Also extract this into a separate function for later use to setup
another driver.
---
 v2v/windows_virtio.ml | 52 ++-
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
index f538b36..b0d9d08 100644
--- a/v2v/windows_virtio.ml
+++ b/v2v/windows_virtio.ml
@@ -82,7 +82,7 @@ let rec install_drivers g inspect systemroot root current_cs 
rcaps =
 let target = sprintf "%s/system32/drivers/viostor.sys" systemroot in
 let target = g#case_sensitive_path target in
 g#cp source target;
-add_viostor_to_registry g inspect root current_cs;
+add_viostor_to_registry g inspect root current_cs driverdir;
 Virtio_blk
 
   | Some IDE, _ ->
@@ -133,11 +133,11 @@ let rec install_drivers g inspect systemroot root 
current_cs rcaps =
 (block, net, video)
   )
 
-and add_viostor_to_registry g inspect root current_cs =
+and add_viostor_to_registry g inspect root current_cs driverdir =
   let { i_major_version = major; i_minor_version = minor;
 i_arch = arch } = inspect in
   if (major == 6 && minor >= 2) || major >= 7 then (* Windows >= 8 *)
-add_viostor_to_driver_database g root arch current_cs
+add_viostor_to_driver_database g root arch current_cs driverdir
   else  (* Windows <= 7 *)
 add_viostor_to_critical_device_database g root current_cs
 
@@ -195,7 +195,7 @@ and add_viostor_to_critical_device_database g root 
current_cs =
 
   reg_import g root regedits
 
-and add_viostor_to_driver_database g root arch current_cs =
+and add_viostor_to_driver_database g root arch current_cs driverdir =
   (* Windows >= 8 doesn't use the CriticalDeviceDatabase.  Instead
* one must add keys into the DriverDatabase.
*)
@@ -213,27 +213,7 @@ and add_viostor_to_driver_database g root arch current_cs =
 sprintf "viostor.inf_%s_%s" arch "c86329aaeb0a7904" in
 
   let scsi_adapter_guid = "{4d36e97b-e325-11ce-bfc1-08002be10318}" in
-  (* There should be a key
-   *   HKLM\SYSTEM\DriverDatabase\DeviceIds\
-   * We want to add:
-   *   "oem1.inf"=hex(0):
-   * but if we find "oem1.inf" we'll add "oem2.inf" (etc).
-   *)
-  let oem_inf =
-let path = [ "DriverDatabase"; "DeviceIds"; scsi_adapter_guid ] in
-match Windows.get_node g root path with
-| None ->
-   error (f_"cannot find HKLM\\SYSTEM\\DriverDatabase\\DeviceIds\\%s in 
the guest registry") scsi_adapter_guid
-| Some node ->
-   let rec loop node i =
- let oem_inf = sprintf "oem%d.inf" i in
- let value = g#hivex_node_get_value node oem_inf in
- if value = 0_L then oem_inf else loop node (i+1)
-   in
-   let oem_inf = loop node 1 in
-   (* Create the key. *)
-   g#hivex_node_set_value node oem_inf (* REG_NONE *) 0_L "";
-   oem_inf in
+  let oem_inf = set_free_oem_inf g root scsi_adapter_guid "viostor.inf" 
driverdir in
 
   (* There should be a key
*   HKLM\SYSTEM\ControlSet001\Control\Class\
@@ -398,6 +378,28 @@ and add_viostor_to_driver_database g root arch current_cs =
@=hex(0012):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\
+ * 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
+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);
+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