Libguestfs - Oct 2015 - Re: [PATCH v2 1/4] v2v: consolidate virtio-win file copying

Copy the appropriate driver files from the virtio-win storage into
%SYSTEMROOT%\Drivers\VirtIO once they are discovered, and stick to using
those copies later on.

This way the knowledge of where the drivers come from originally is
consolidated in one place, so the lifetime of the associated entities
becomes easier to control (to be implemented in followup patches).

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
changes since v1:
 - former patch #2

 v2v/convert_windows.ml | 128 ++++++++++++++++++++++---------------------------
 1 file changed, 56 insertions(+), 72 deletions(-)

diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
index d1aef36..55213aa 100644
--- a/v2v/convert_windows.ml
+++ b/v2v/convert_windows.ml
@@ -271,12 +271,9 @@ echo uninstalling Xen PV driver
     with
       Not_found -> ()
 
-  and install_virtio_drivers root current_cs -    (* Copy the virtio drivers to
the guest. *)
-    let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in
-    g#mkdir_p driverdir;
+  and copy_virtio_drivers driverdir +    (* Copy the matching drivers to the
driverdir. *)
 
-    (* Load the list of drivers available. *)
     let drivers = find_virtio_win_drivers virtio_win in
 
     (* Filter out only drivers matching the current guest. *)
@@ -300,73 +297,60 @@ echo uninstalling Xen PV driver
       flush stdout
     );
 
-    match drivers with
-    | [] ->
-       warning (f_"there are no virtio drivers available for this version
of Windows (%d.%d %s %s).  virt-v2v looks for drivers in %s\n\nThe guest will be
configured to use slower emulated devices.")
-               inspect.i_major_version inspect.i_minor_version
-               inspect.i_arch inspect.i_product_variant
-               virtio_win;
-       ( IDE, RTL8139, Cirrus )
-
-    | drivers ->
-       (* Can we install the block driver? *)
-       let block : guestcaps_block_type -         try
-           let viostor_sys_file -             List.find
-               (fun { vwd_filename = filename } -> filename =
"viostor.sys")
-               drivers in
-           (* Get the actual file contents of the .sys file. *)
-           let content = viostor_sys_file.vwd_get_contents () in
-           let target = sprintf "%s/system32/drivers/viostor.sys"
systemroot in
-           let target = g#case_sensitive_path target in
-           g#write target content;
-           add_viostor_to_critical_device_database root current_cs;
-           Virtio_blk
-         with Not_found ->
-           warning (f_"there is no viostor (virtio block device) driver
for this version of Windows (%d.%d %s).  virt-v2v looks for this driver in
%s\n\nThe guest will be configured to use a slower emulated device.")
-                   inspect.i_major_version inspect.i_minor_version
-                   inspect.i_arch virtio_win;
-           IDE in
-
-       (* Can we install the virtio-net driver? *)
-       let net : guestcaps_net_type -         if not (List.exists
-                   (fun { vwd_filename = filename } -> filename =
"netkvm.inf")
-                   drivers) then (
-           warning (f_"there is no virtio network driver for this version
of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe guest will
be configured to use a slower emulated device.")
-                   inspect.i_major_version inspect.i_minor_version
-                   inspect.i_arch virtio_win;
-           RTL8139
-         )
-         else
-           (* It will be installed at firstboot. *)
-           Virtio_net in
-
-       (* Can we install the QXL driver? *)
-       let video : guestcaps_video_type -         if not (List.exists
-                   (fun { vwd_filename = filename } -> filename =
"qxl.inf")
-                   drivers) then (
-           warning (f_"there is no QXL driver for this version of Windows
(%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe guest will be
configured to use standard VGA.")
-                   inspect.i_major_version inspect.i_minor_version
-                   inspect.i_arch virtio_win;
-           Cirrus
-         )
-         else
-           (* It will be installed at firstboot. *)
-           QXL in
-
-       (* Copy all the drivers to the driverdir.  They will be
-        * installed at firstboot.
-        *)
-       List.iter (
-         fun driver ->
-           let content = driver.vwd_get_contents () in
-           g#write (driverdir // driver.vwd_filename) content
-       ) drivers;
-
-       (block, net, video)
+    List.iter (
+      fun driver ->
+        let content = driver.vwd_get_contents () in
+        g#write (driverdir // driver.vwd_filename) content
+    ) drivers
+
+  and install_virtio_drivers root current_cs +    (* Copy the virtio drivers to
the guest. *)
+    let driverdir = sprintf "%s/Drivers/VirtIO" systemroot in
+    g#mkdir_p driverdir;
+
+    copy_virtio_drivers driverdir;
+
+    (* Can we install the block driver? *)
+    let block : guestcaps_block_type +      let source = driverdir //
"viostor.sys" in
+      if (g#exists source) then (
+        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_critical_device_database root current_cs;
+        Virtio_blk
+      ) else (
+        warning (f_"there is no viostor (virtio block device) driver for
this version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
+                inspect.i_major_version inspect.i_minor_version
+                inspect.i_arch virtio_win;
+        IDE
+      ) in
+
+    (* Can we install the virtio-net driver? *)
+    let net : guestcaps_net_type +      if not (g#exists (driverdir //
"netkvm.inf")) then (
+        warning (f_"there is no virtio network driver for this version of
Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe guest will be
configured to use a slower emulated device.")
+                inspect.i_major_version inspect.i_minor_version
+                inspect.i_arch virtio_win;
+        RTL8139
+      )
+      else
+        (* It will be installed at firstboot. *)
+        Virtio_net in
+
+    (* Can we install the QXL driver? *)
+    let video : guestcaps_video_type +      if not (g#exists (driverdir //
"qxl.inf")) then (
+        warning (f_"there is no QXL driver for this version of Windows
(%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe guest will be
configured to use standard VGA.")
+                inspect.i_major_version inspect.i_minor_version
+                inspect.i_arch virtio_win;
+        Cirrus
+      )
+      else
+        (* It will be installed at firstboot. *)
+        QXL in
+
+    (block, net, video)
 
   and add_viostor_to_critical_device_database root current_cs      let {
i_major_version = major; i_minor_version = minor;
-- 
2.4.3

On Tue, Oct 13, 2015 at 03:50:51PM +0300, Roman Kagan
wrote:
>  
> -    match drivers with
> -    | [] ->
> -       warning (f_"there are no virtio drivers available for this
version of Windows (%d.%d %s %s).  virt-v2v looks for drivers in %s\n\nThe guest
will be configured to use slower emulated devices.")
> -               inspect.i_major_version inspect.i_minor_version
> -               inspect.i_arch inspect.i_product_variant
> -               virtio_win;
> -       ( IDE, RTL8139, Cirrus )
I believe this warning gets lost in this patch.

To get it back, let's make 'copy_virtio_drivers' returns a
true/false
indication ('true' meaning that at least some drivers were copied),
and the caller code in 'install_virtio_drivers' should check this, and
either do the code above, or continue with the code below.
> +    copy_virtio_drivers driverdir;
> +
> +    (* Can we install the block driver? *)
> +    let block : guestcaps_block_type > +      let source = driverdir //
"viostor.sys" in
> +      if (g#exists source) then (
Don't need parentheses around if-conditions.

The rest of the patch looks good with these two changes.

Rich.

-- 
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, Oct 13, 2015 at 03:16:27PM +0100, Richard W.M. Jones
wrote:
> On Tue, Oct 13, 2015 at 03:50:51PM +0300, Roman Kagan wrote:
> >  
> > -    match drivers with
> > -    | [] ->
> > -       warning (f_"there are no virtio drivers available for
this version of Windows (%d.%d %s %s).  virt-v2v looks for drivers in %s\n\nThe
guest will be configured to use slower emulated devices.")
> > -               inspect.i_major_version inspect.i_minor_version
> > -               inspect.i_arch inspect.i_product_variant
> > -               virtio_win;
> > -       ( IDE, RTL8139, Cirrus )
> 
> I believe this warning gets lost in this patch.
> 
> To get it back, let's make 'copy_virtio_drivers' returns a
true/false
> indication ('true' meaning that at least some drivers were copied),
> and the caller code in 'install_virtio_drivers' should check this,
and
> either do the code above, or continue with the code below.
Is there any difference between not being able to locate some of the
virtio drivers or all of them?

With this patch, you'll get an individual warning for every one of the
missing drivers.  I don't see why we need an extra case for "all
drivers
missing" (for any particular value of "all").

Roman.