Libguestfs - Oct 2015 - Re: [PATCH v3 0/3] v2v: simplify driver copying from virtio-win iso

Libguestfs supports passing an ISO image as a source of virtio windows
drivers to v2v.

This series attempts to make it simpler and better scoped.

Roman Kagan (3):
  v2v: consolidate virtio-win file copying
  v2v: copy virtio drivers without guestfs handle leak
  v2v: drop useless forced gc

---
changes since v2:
 - drop patch 4 (reuse of the master guestfs handle for hot-adding the
   ISO image)
 - more excessive parentheses removed
 - revert the removal of a warning when no virtio drivers matching the
   guest OS are found
 - revert the undesired removal of guestfs handle identifier assignment

changes since v1:
 - rebased to latest master (in particular, updated usage of string
   functions)
 - add missing and remove excessive parentheses
 - reorder patches
 - preserve support for backends with no hot-add capability

 v2v/convert_windows.ml | 192 +++++++++++++++++++++++---------------------
 v2v/utils.ml           | 213 +++++++++++++++----------------------------------
 v2v/v2v.ml             |   8 --
 3 files changed, 167 insertions(+), 246 deletions(-)

-- 
2.4.3

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 v2:
 - more excessive parentheses removed
 - revert the removal of a warning when no virtio drivers matching the
   guest OS are found

changes since v1:
 - former patch #2

 v2v/convert_windows.ml | 139 +++++++++++++++++++++++--------------------------
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
index d1aef36..c103d7c 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;
-
-    (* Load the list of drivers available. *)
+  and copy_virtio_drivers driverdir +    (* Copy the matching drivers to the
driverdir; return true if any have been
+     * copied *)
     let drivers = find_virtio_win_drivers virtio_win in
 
     (* Filter out only drivers matching the current guest. *)
@@ -300,73 +297,69 @@ 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)
+    let ret = ref false in
+    List.iter (
+      fun driver ->
+        let content = driver.vwd_get_contents () in
+        g#write (driverdir // driver.vwd_filename) content;
+        ret := true
+    ) drivers;
+    !ret
+
+  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;
+
+    if not (copy_virtio_drivers driverdir) then (
+      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 )
+    )
+    else (
+      (* 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

Refactor copying of virtio windows drivers into the guest so that the
matching of the drivers to the guest os flavor and copying the files
happens one next to the other in a single function, and no guestfs
handle (nor any other irrelevant info) is leaked outside.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
changes since v2:
 - more excessive parentheses removed
 - revert the removal of a warning when no virtio drivers matching the
   guest OS are found
 - revert the undesired removal of guestfs handle identifier assignment

changes since v1:
 - former patch #3
 - updated usage of string functions

 v2v/convert_windows.ml |  77 +++++++++++-------
 v2v/utils.ml           | 213 +++++++++++++++----------------------------------
 2 files changed, 113 insertions(+), 177 deletions(-)

diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
index c103d7c..ed2542e 100644
--- a/v2v/convert_windows.ml
+++ b/v2v/convert_windows.ml
@@ -274,36 +274,55 @@ echo uninstalling Xen PV driver
   and copy_virtio_drivers driverdir      (* Copy the matching drivers to the
driverdir; return true if any have been
      * copied *)
-    let drivers = find_virtio_win_drivers virtio_win in
-
-    (* Filter out only drivers matching the current guest. *)
-    let drivers -      List.filter (
-        fun { vwd_os_arch = arch;
-              vwd_os_major = os_major; vwd_os_minor = os_minor;
-              vwd_os_variant = os_variant } ->
-        arch = inspect.i_arch &&
-        os_major = inspect.i_major_version &&
-        os_minor = inspect.i_minor_version &&
-        (match os_variant with
-         | Vwd_client -> inspect.i_product_variant = "Client"
-         | Vwd_not_client -> inspect.i_product_variant <>
"Client"
-         | Vwd_any_variant -> true)
-      ) drivers in
-
-    if verbose () then (
-      printf "virtio-win driver files matching this guest:\n";
-      List.iter print_virtio_win_driver_file drivers;
-      flush stdout
-    );
-
     let ret = ref false in
-    List.iter (
-      fun driver ->
-        let content = driver.vwd_get_contents () in
-        g#write (driverdir // driver.vwd_filename) content;
-        ret := true
-    ) drivers;
+    if is_directory virtio_win then (
+      let cmd = sprintf "cd %s && find -type f" (quote
virtio_win) in
+      let paths = external_command cmd in
+      List.iter (
+        fun path ->
+          if match_vio_path_with_os path inspect then (
+            let source = virtio_win // path in
+            let target = driverdir //
+                    String.lowercase_ascii (Filename.basename path) in
+            if verbose () then
+              printf "Copying virtio driver bits: 'host:%s' ->
'%s'\n"
+                source target;
+
+            g#write target (read_whole_file source);
+            ret := true
+          )
+      ) paths
+    )
+    else if is_regular_file virtio_win then (
+      try
+        let g2 = new Guestfs.guestfs () in
+        g#set_identifier "virtio_win";
+        if trace () then g2#set_trace true;
+        if verbose () then g2#set_verbose true;
+        g2#add_drive_opts virtio_win ~readonly:true;
+        g2#launch ();
+        let vio_root = "/" in
+        g2#mount_ro "/dev/sda" vio_root;
+        let paths = g2#find vio_root in
+        Array.iter (
+          fun path ->
+            let source = vio_root // path in
+            if g2#is_file source ~followsymlinks:false &&
+               match_vio_path_with_os path inspect then (
+              let target = driverdir //
+                      String.lowercase_ascii (Filename.basename path) in
+              if verbose () then
+                printf "Copying virtio driver bits: '%s:%s' ->
'%s'\n"
+                  virtio_win path target;
+
+              g#write target (g2#read_file source);
+              ret := true
+            )
+        ) paths;
+        g2#close()
+      with Guestfs.Error msg ->
+        error (f_"%s: cannot open virtio-win ISO file: %s")
virtio_win msg
+    );
     !ret
 
   and install_virtio_drivers root current_cs diff --git a/v2v/utils.ml
b/v2v/utils.ml
index 2998d90..8aaac09 100644
--- a/v2v/utils.ml
+++ b/v2v/utils.ml
@@ -159,161 +159,78 @@ let find_uefi_firmware guest_arch    in
   loop files
 
-(* Find virtio-win driver files from an unpacked or mounted virtio-win
- * directory, or from a virtio-win.iso file. The location of drivers
-  varies between releases of virtio-win and also across Fedora and
-  RHEL so try to be robust to changes.
- *)
-type virtio_win_driver_file = {
-  (* Base filename, eg. "netkvm.sys".  Always lowercase. *)
-  vwd_filename : string;
-  (* Return the contents of this file. *)
-  vwd_get_contents : unit -> string;
-
-  (* Various fields that classify this driver: *)
-
-  vwd_os_major : int;           (* Windows version. *)
-  vwd_os_minor : int;
-  vwd_os_variant : vwd_os_variant;
-  vwd_os_arch : string;         (* Architecture, eg "i386",
"x86_64". *)
-  vwd_extension : string;       (* File extension (lowercase), eg.
"sys" "inf"*)
+let is_regular_file path = (* NB: follows symlinks. *)
+  try (Unix.stat path).Unix.st_kind = Unix.S_REG
+  with Unix.Unix_error _ -> false
 
-  (* Original source of file (for debugging only). *)
-  vwd_original_source : string;
-}
-and vwd_os_variant = Vwd_client | Vwd_not_client | Vwd_any_variant
-
-let print_virtio_win_driver_file vwd -  printf "%s [%d,%d,%s,%s,%s] from
%s\n"
-         vwd.vwd_filename
-         vwd.vwd_os_major vwd.vwd_os_minor
-         (match vwd.vwd_os_variant with
-          | Vwd_client -> "client" | Vwd_not_client ->
"not-client"
-          | Vwd_any_variant -> "any")
-         vwd.vwd_os_arch
-         vwd.vwd_extension
-         vwd.vwd_original_source
-
-let find_virtio_win_drivers virtio_win -  let is_regular_file path = (* NB:
follows symlinks. *)
-    try (Unix.stat path).Unix.st_kind = Unix.S_REG
-    with Unix.Unix_error _ -> false
-  in
-
-  let files -    if is_directory virtio_win then (
-      let cmd = sprintf "cd %s && find -type f" (quote
virtio_win) in
-      let paths = external_command cmd in
-      List.map (
-        fun path ->
-          let abs_path = virtio_win // path in
-          (path, abs_path,
-           Filename.basename path,
-           fun () -> read_whole_file abs_path)
-      ) paths
-    )
-    else if is_regular_file virtio_win then (
-      try
-        let g = new Guestfs.guestfs () in
-        g#set_identifier "virtio_win";
-        if trace () then g#set_trace true;
-        if verbose () then g#set_verbose true;
-        g#add_drive_opts virtio_win ~readonly:true;
-        g#launch ();
-        g#mount_ro "/dev/sda" "/";
-        let paths = g#find "/" in
-        let paths = Array.to_list paths in
-        let paths = List.map ((^) "/") paths in
-        let paths = List.filter (g#is_file ~followsymlinks:false) paths in
-        List.map (
-          fun path ->
-            let basename -              match last_part_of path '/'
with
-              | Some x -> x
-              | None ->
-                error "v2v/find_virtio_win_drivers: missing '/' in
%s" path in
-            (path, sprintf "%s:%s" virtio_win path,
-             basename,
-             fun () -> g#read_file path)
-        ) paths
-      with Guestfs.Error msg ->
-        error (f_"%s: cannot open virtio-win ISO file: %s")
virtio_win msg
-    )
-    else [] in
-
-  let files -    filter_map (
-      fun (path, original_source, basename, get_contents) ->
-        try
-          (* Lowercased path, since the ISO may contain upper or lowercase
-           * path elements.
-           *)
-          let lc_path = String.lowercase_ascii path in
-          let lc_basename = String.lowercase_ascii basename in
+(* Given a path of a file relative to the root of the directory tree with
+ * virtio-win drivers, figure out if it's suitable for the specific Windows
+ * flavor.
+ *)
+let match_vio_path_with_os path inspect +  let { i_major_version = os_major;
i_minor_version = os_minor;
+        i_arch = arch; i_product_variant = os_variant } = inspect in
+  try
+    (* Lowercased path, since the ISO may contain upper or lowercase path
+     * elements. *)
+    let lc_path = String.lowercase_ascii path in
+    let lc_basename = Filename.basename path in
 
-          let extension -            match last_part_of lc_basename '.'
with
-            | Some x -> x
-            | None -> raise Not_found
-          in
+    let extension +      match last_part_of lc_basename '.' with
+      | Some x -> x
+      | None -> raise Not_found
+    in
 
-          (* Skip files without specific extensions. *)
-          if extension <> "cat" && extension <>
"inf" &&
-               extension <> "pdb" && extension <>
"sys" then
-            raise Not_found;
+    (* Skip files without specific extensions. *)
+    let extensions = ["cat"; "inf"; "pdb";
"sys"] in
+    if not (List.mem extension extensions) then raise Not_found;
 
-          (* Using the full path, work out what version of Windows
-           * this driver is for.  Paths can be things like:
-           * "NetKVM/2k12R2/amd64/netkvm.sys" or
-           * "./drivers/amd64/Win2012R2/netkvm.sys".
-           * Note we check lowercase paths.
-           *)
-          let pathelem elem = String.find lc_path ("/" ^ elem ^
"/") >= 0 in
-          let arch -            if pathelem "x86" || pathelem
"i386" then "i386"
-            else if pathelem "amd64" then "x86_64"
-            else raise Not_found in
-          let os_major, os_minor, os_variant -            if pathelem
"xp" || pathelem "winxp" then
-              (5, 1, Vwd_any_variant)
-            else if pathelem "2k3" || pathelem "win2003"
then
-              (5, 2, Vwd_any_variant)
-            else if pathelem "vista" then
-              (6, 0, Vwd_client)
-            else if pathelem "2k8" || pathelem "win2008"
then
-              (6, 0, Vwd_not_client)
-            else if pathelem "w7" || pathelem "win7" then
-              (6, 1, Vwd_client)
-            else if pathelem "2k8r2" || pathelem
"win2008r2" then
-              (6, 1, Vwd_not_client)
-            else if pathelem "w8" || pathelem "win8" then
-              (6, 2, Vwd_client)
-            else if pathelem "2k12" || pathelem "win2012"
then
-              (6, 2, Vwd_not_client)
-            else if pathelem "w8.1" || pathelem "win8.1"
then
-              (6, 3, Vwd_client)
-            else if pathelem "2k12r2" || pathelem
"win2012r2" then
-              (6, 3, Vwd_not_client)
-            else if pathelem "w10" || pathelem "win10" then
-              (10, 0, Vwd_client)
-            else
-              raise Not_found in
+    (* Using the full path, work out what version of Windows
+     * this driver is for.  Paths can be things like:
+     * "NetKVM/2k12R2/amd64/netkvm.sys" or
+     * "./drivers/amd64/Win2012R2/netkvm.sys".
+     * Note we check lowercase paths.
+     *)
+    let pathelem elem = String.find lc_path ("/" ^ elem ^
"/") >= 0 in
+    let p_arch +      if pathelem "x86" || pathelem "i386"
then "i386"
+      else if pathelem "amd64" then "x86_64"
+      else raise Not_found in
 
-          Some {
-            vwd_filename = lc_basename;
-            vwd_get_contents = get_contents;
-            vwd_os_major = os_major;
-            vwd_os_minor = os_minor;
-            vwd_os_variant = os_variant;
-            vwd_os_arch = arch;
-            vwd_extension = extension;
-            vwd_original_source = original_source;
-          }
+    let is_client os_variant = os_variant = "Client"
+    and not_client os_variant = os_variant <> "Client"
+    and any_variant os_variant = true in
+    let p_os_major, p_os_minor, match_os_variant +      if pathelem
"xp" || pathelem "winxp" then
+        (5, 1, any_variant)
+      else if pathelem "2k3" || pathelem "win2003" then
+        (5, 2, any_variant)
+      else if pathelem "vista" then
+        (6, 0, is_client)
+      else if pathelem "2k8" || pathelem "win2008" then
+        (6, 0, not_client)
+      else if pathelem "w7" || pathelem "win7" then
+        (6, 1, is_client)
+      else if pathelem "2k8r2" || pathelem "win2008r2" then
+        (6, 1, not_client)
+      else if pathelem "w8" || pathelem "win8" then
+        (6, 2, is_client)
+      else if pathelem "2k12" || pathelem "win2012" then
+        (6, 2, not_client)
+      else if pathelem "w8.1" || pathelem "win8.1" then
+        (6, 3, is_client)
+      else if pathelem "2k12r2" || pathelem "win2012r2"
then
+        (6, 3, not_client)
+      else if pathelem "w10" || pathelem "win10" then
+        (10, 0, is_client)
+      else
+        raise Not_found in
 
-        with Not_found -> None
-    ) files in
+    arch = p_arch && os_major = p_os_major && os_minor =
p_os_minor &&
+      match_os_variant os_variant
 
-  files
+  with Not_found -> false
 
 let compare_app2_versions app1 app2    let i = compare app1.Guestfs.app2_epoch
app2.Guestfs.app2_epoch in
-- 
2.4.3

Now that virtio driver copying is localized in a single function and the
auxiliary guestfs handle doesn't leak outside of it (and thus an extra
qemu instance doesn't survive beyond that function's runtime)
there's no
need in the forced GC which used to trigger closing of that handle and
termination of that qemu instance.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
no changes since v2

changes since v1:
 - former patch #1

 v2v/v2v.ml | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/v2v/v2v.ml b/v2v/v2v.ml
index 4d621b8..fe16131 100644
--- a/v2v/v2v.ml
+++ b/v2v/v2v.ml
@@ -319,14 +319,6 @@ let rec main ()    if verbose () then
     printf "%s%!" (string_of_target_buses target_buses);
 
-  (* Force a GC here, to ensure that we're using the minimum resources
-   * as we go into the copy stage.  The particular reason is that
-   * Windows conversion may have opened a second libguestfs handle
-   * pointing to the virtio-win ISO, which is only closed when the
-   * handle is GC'd.
-   *)
-  Gc.compact ();
-
   let delete_target_on_exit = ref true in
 
   let targets -- 
2.4.3

On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan
wrote:
> Libguestfs supports passing an ISO image as a source of virtio windows
> drivers to v2v.
> 
> This series attempts to make it simpler and better scoped.
Looks good (apart from patch 3 - the Gc.compact is necessary!)

I'm just running the tests and will push it later if that goes well.

Thanks,
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Thu, Oct 15, 2015 at 10:07:45AM +0100, Richard W.M. Jones
wrote:
> On Wed, Oct 14, 2015 at 07:55:41PM +0300, Roman Kagan wrote:
> > Libguestfs supports passing an ISO image as a source of virtio windows
> > drivers to v2v.
> > 
> > This series attempts to make it simpler and better scoped.
> 
> Looks good (apart from patch 3 - the Gc.compact is necessary!)
Why?  As a matter of fact that was the primary reason I even started
with this patchset: that Gc.compact was sitting in the middle of the
main v2v function and was yet another obstacle in the refactoring I was
after.

Now that the extra guestfs handle doesn't survive beyond
copy_virtio_drivers() what is the need for that explicit GC (which was
introduced at the time exactly to clear up that handle in commit
47b5f245bec908f803f0a89c3b1e3166cfe33aad)?

Roman.