Libguestfs - Jan 2019 - Re: [PATCH 1/2] v2v: fix path to source when copying files from guest tools directory

First patch just fixes installing guest tools from directory that was broken.
Second patch revamps how virt-v2v chooses from which directory install guest
tools on Linux. Details in commit message.

Tomáš Golembiovský (2):
  v2v: fix path to source when copying files from guest tools directory
  v2v: allow alternative directories for distributions

 v2v/windows_virtio.ml | 67 +++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

-- 
2.20.1

The debug message was slightly changed too to better match the similar
message for ISO case. It refers to the root directory instead of the
specific subdirectory inside guest tools.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 v2v/windows_virtio.ml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
index 92bf3ec60..94c4774b7 100644
--- a/v2v/windows_virtio.ml
+++ b/v2v/windows_virtio.ml
@@ -308,10 +308,10 @@ and copy_drivers g inspect driverdir  and
copy_from_virtio_win g inspect srcdir destdir filter missing    let ret = ref []
in
   if is_directory virtio_win then (
-    let dir = virtio_win // srcdir in
-    debug "windows: copy_from_virtio_win: guest tools source directory
%s" dir;
+    debug "windows: copy_from_virtio_win: guest tools source directory
%s" virtio_win;
 
-    if not (is_directory srcdir) then missing ()
+    let dir = virtio_win // srcdir in
+    if not (is_directory dir) then missing ()
     else (
       let cmd = sprintf "cd %s && find -L -type f" (quote
dir) in
       let paths = external_command cmd in
-- 
2.20.1

Allow multiple alternative directory names for distributions (or
distribution familiy) when installing Linux guest tools packages.
Original naming required that there is a separate directory for every
version of a distribution (e.g. fc28, fc29, ...). This is inconvenient
when users want to keep just a single version of the package for the
distribution.

For each distribution one can have either a common directory (e.g.
fedora) or a versioned directory (fedora28). This can also be combined.
I.e. one can have both `fedora` and `fedora28` in which case `fedora28`
will be used when converting Fedora 28 guest wheres `fedora` will be
used when converting guests with any other Fedora version.

To have better names for unversioned directories the original names
were changed this way:

    fc -> fedora
    el -> rhel
    lp -> suse

The original directory names are kept for backward compatibility and are
aliased to new names as described below. When both new and old name are
present on file system the new name takes precedence.

    fc28 -> fedora
    el6 -> rhel6
    el7 -> rhel7
    lp151 -> suse

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 v2v/windows_virtio.ml | 65 +++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
index 94c4774b7..cc33d9502 100644
--- a/v2v/windows_virtio.ml
+++ b/v2v/windows_virtio.ml
@@ -186,14 +186,18 @@ let rec install_drivers ((g, _) as reg) inspect rcaps  and
install_linux_tools g inspect    let os      match inspect.i_distro with
-    | "fedora" -> Some "fc28"
+    | "fedora" -> Some [
+      (sprintf "fedora%d" inspect.i_major_version);
"fedora"; "fc28"]
     | "rhel" | "centos" | "scientificlinux" |
"redhat-based"
-    | "oraclelinux" ->
-      (match inspect.i_major_version with
-       | 6 -> Some "el6"
-       | 7 -> Some "el7"
-       | _ -> None)
-    | "sles" | "suse-based" | "opensuse" ->
Some "lp151"
+    | "oraclelinux" -> Some (
+      [(sprintf "rhel%d" inspect.i_major_version)]
+      @ (match inspect.i_major_version with
+       | 6 -> ["el6"]
+       | 7 -> ["el7"]
+       | _ -> [])
+      @ ["rhel"])
+    | "sles" | "suse-based" | "opensuse" ->
Some [
+      (sprintf "fedora%d" inspect.i_major_version); "suse";
"lp151"]
     | _ -> None in
 
   match os with
@@ -201,15 +205,15 @@ and install_linux_tools g inspect        warning
(f_"don't know how to install guest tools on %s-%d")
         inspect.i_distro inspect.i_major_version
   | Some os ->
-      let src_path = "linux" // os in
+      let src_paths = List.map ((//) "linux") os in
       let dst_path = "/var/tmp" in
-      debug "locating packages in %s" src_path;
+      debug "locating packages in: %s" (String.concat ", "
src_paths);
       let packages -        copy_from_virtio_win g inspect src_path dst_path
+        copy_from_virtio_win g inspect src_paths dst_path
                              (fun _ _ -> true)
                              (fun () ->
-                               warning (f_"guest tools directory ‘%s’ is
missing from the virtio-win directory or ISO.\n\nGuest tools are only provided
in the RHV Guest Tools ISO, so this can happen if you are using the version of
virtio-win which contains just the virtio drivers.  In this case only virtio
drivers can be installed in the guest, and installation of Guest Tools will be
skipped.")
-                                       src_path) in
+                               warning (f_"none of the guest tools
directories ‘[%s]’ was found on the virtio-win directory or ISO.\n\nGuest tools
are only provided in the oVirt/RHV Guest Tools ISO, so this can happen if you
are using the version of virtio-win which contains just the virtio drivers.  In
this case only virtio drivers can be installed in the guest, and installation of
Guest Tools will be skipped.")
+                                  (String.concat ", " src_paths)) in
       debug "done copying %d files" (List.length packages);
       let packages = List.map ((//) dst_path) packages in
       try
@@ -290,29 +294,34 @@ and ddb_regedits inspect drv_name drv_pciid   * been
copied.
  *)
 and copy_drivers g inspect driverdir -  [] <> copy_from_virtio_win g
inspect "/" driverdir
+  [] <> copy_from_virtio_win g inspect ["/"] driverdir
     virtio_iso_path_matches_guest_os
     (fun () ->
       error (f_"root directory ‘/’ is missing from the virtio-win
directory or ISO.\n\nThis should not happen and may indicate that virtio-win or
virt-v2v is broken in some way.  Please report this as a bug with a full debug
log."))
 
-(* Copy all files from virtio_win directory/ISO located in [srcdir]
- * subdirectory and all its subdirectories to the [destdir]. The directory
- * hierarchy is not preserved, meaning all files will be directly in [destdir].
- * The file list is filtered based on [filter] function.
+(* Find first existing direcotry from [scrdirs] list located in virtio_win
+ * directory/ISO and copy all files from that directory and all its
+ * subdirectories to the [destdir]. The directory hierarchy is not preserved,
+ * meaning all files will be directly in [destdir]. The file list is filtered
+ * based on [filter] function.
  *
- * If [srcdir] is missing from the ISO then [missing ()] is called
+ * If none of the directories in [srcdirs] exists [missing ()] is called
  * which might give a warning or error.
  *
+ * Note that the call may succeed whithout copying any file at all. This may
+ * happen when the source subdirectory exists but is empty or when [filter]
+ * function is too strict to allow any of the files.
+ *
  * Returns list of copied files.
  *)
-and copy_from_virtio_win g inspect srcdir destdir filter missing +and
copy_from_virtio_win g inspect srcdirs destdir filter missing    let ret = ref
[] in
   if is_directory virtio_win then (
     debug "windows: copy_from_virtio_win: guest tools source directory
%s" virtio_win;
 
-    let dir = virtio_win // srcdir in
-    if not (is_directory dir) then missing ()
-    else (
+    try
+      let srcdirs = List.map ((//) virtio_win) srcdirs in
+      let dir = List.find is_directory srcdirs in
       let cmd = sprintf "cd %s && find -L -type f" (quote
dir) in
       let paths = external_command cmd in
       List.iter (
@@ -328,7 +337,8 @@ and copy_from_virtio_win g inspect srcdir destdir filter
missing              List.push_front target_name ret
           )
       ) paths
-    )
+    with Not_found ->
+      missing()
   )
   else if is_regular_file virtio_win then (
     debug "windows: copy_from_virtio_win: guest tools source ISO %s"
virtio_win;
@@ -339,9 +349,9 @@ and copy_from_virtio_win g inspect srcdir destdir filter
missing        g2#launch ();
       let vio_root = "/" in
       g2#mount_ro "/dev/sda" vio_root;
-      let srcdir = vio_root ^ "/" ^ srcdir in
-      if not (g2#is_dir srcdir) then missing ()
-      else (
+      let srcdirs = List.map ((//) vio_root) srcdirs in
+      try
+        let srcdir = List.find g2#is_dir srcdirs in
         let paths = g2#find srcdir in
         Array.iter (
           fun path ->
@@ -357,7 +367,8 @@ and copy_from_virtio_win g inspect srcdir destdir filter
missing                List.push_front target_name ret
             )
         ) paths;
-      );
+      with Not_found ->
+        missing();
       g2#close()
     with Guestfs.Error msg ->
       error (f_"%s: cannot open virtio-win ISO file: %s") virtio_win
msg
-- 
2.20.1

Il giorno sab 26 gen 2019, 13:20 Tomáš Golembiovský <tgolembi@redhat.com>
ha scritto:
> Allow multiple alternative directory names for distributions (or
> distribution familiy) when installing Linux guest tools packages.
> Original naming required that there is a separate directory for every
> version of a distribution (e.g. fc28, fc29, ...). This is inconvenient
> when users want to keep just a single version of the package for the
> distribution.
>
> For each distribution one can have either a common directory (e.g.
> fedora) or a versioned directory (fedora28). This can also be combined.
> I.e. one can have both `fedora` and `fedora28` in which case `fedora28`
> will be used when converting Fedora 28 guest wheres `fedora` will be
> used when converting guests with any other Fedora version.
>
> To have better names for unversioned directories the original names
> were changed this way:
>
>     fc -> fedora
>     el -> rhel
>

this will cut centos off



    lp -> suse
>
> The original directory names are kept for backward compatibility and are
> aliased to new names as described below. When both new and old name are
> present on file system the new name takes precedence.
>
>     fc28 -> fedora
>     el6 -> rhel6
>     el7 -> rhel7
>     lp151 -> suse
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  v2v/windows_virtio.ml | 65 +++++++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index 94c4774b7..cc33d9502 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -186,14 +186,18 @@ let rec install_drivers ((g, _) as reg) inspect
> rcaps >  and install_linux_tools g inspect >    let os >     
match inspect.i_distro with
> -    | "fedora" -> Some "fc28"
> +    | "fedora" -> Some [
> +      (sprintf "fedora%d" inspect.i_major_version);
"fedora"; "fc28"]
>      | "rhel" | "centos" | "scientificlinux"
| "redhat-based"
> -    | "oraclelinux" ->
> -      (match inspect.i_major_version with
> -       | 6 -> Some "el6"
> -       | 7 -> Some "el7"
> -       | _ -> None)
> -    | "sles" | "suse-based" | "opensuse"
-> Some "lp151"
> +    | "oraclelinux" -> Some (
> +      [(sprintf "rhel%d" inspect.i_major_version)]
> +      @ (match inspect.i_major_version with
> +       | 6 -> ["el6"]
> +       | 7 -> ["el7"]
> +       | _ -> [])
> +      @ ["rhel"])
> +    | "sles" | "suse-based" | "opensuse"
-> Some [
> +      (sprintf "fedora%d" inspect.i_major_version);
"suse"; "lp151"]
>      | _ -> None in
>
>    match os with
> @@ -201,15 +205,15 @@ and install_linux_tools g inspect >        warning
(f_"don't know how to install guest tools on %s-%d")
>          inspect.i_distro inspect.i_major_version
>    | Some os ->
> -      let src_path = "linux" // os in
> +      let src_paths = List.map ((//) "linux") os in
>        let dst_path = "/var/tmp" in
> -      debug "locating packages in %s" src_path;
> +      debug "locating packages in: %s" (String.concat ",
" src_paths);
>        let packages > -        copy_from_virtio_win g inspect src_path
dst_path
> +        copy_from_virtio_win g inspect src_paths dst_path
>                               (fun _ _ -> true)
>                               (fun () ->
> -                               warning (f_"guest tools directory ‘%s’
is
> missing from the virtio-win directory or ISO.\n\nGuest tools are only
> provided in the RHV Guest Tools ISO, so this can happen if you are using
> the version of virtio-win which contains just the virtio drivers.  In this
> case only virtio drivers can be installed in the guest, and installation of
> Guest Tools will be skipped.")
> -                                       src_path) in
> +                               warning (f_"none of the guest tools
> directories ‘[%s]’ was found on the virtio-win directory or ISO.\n\nGuest
> tools are only provided in the oVirt/RHV Guest Tools ISO, so this can
> happen if you are using the version of virtio-win which contains just the
> virtio drivers.  In this case only virtio drivers can be installed in the
> guest, and installation of Guest Tools will be skipped.")
> +                                  (String.concat ", "
src_paths)) in
>        debug "done copying %d files" (List.length packages);
>        let packages = List.map ((//) dst_path) packages in
>        try
> @@ -290,29 +294,34 @@ and ddb_regedits inspect drv_name drv_pciid >   *
been copied.
>   *)
>  and copy_drivers g inspect driverdir > -  [] <>
copy_from_virtio_win g inspect "/" driverdir
> +  [] <> copy_from_virtio_win g inspect ["/"] driverdir
>      virtio_iso_path_matches_guest_os
>      (fun () ->
>        error (f_"root directory ‘/’ is missing from the virtio-win
> directory or ISO.\n\nThis should not happen and may indicate that
> virtio-win or virt-v2v is broken in some way.  Please report this as a bug
> with a full debug log."))
>
> -(* Copy all files from virtio_win directory/ISO located in [srcdir]
> - * subdirectory and all its subdirectories to the [destdir]. The directory
> - * hierarchy is not preserved, meaning all files will be directly in
> [destdir].
> - * The file list is filtered based on [filter] function.
> +(* Find first existing direcotry from [scrdirs] list located in virtio_win
> + * directory/ISO and copy all files from that directory and all its
> + * subdirectories to the [destdir]. The directory hierarchy is not
> preserved,
> + * meaning all files will be directly in [destdir]. The file list is
> filtered
> + * based on [filter] function.
>   *
> - * If [srcdir] is missing from the ISO then [missing ()] is called
> + * If none of the directories in [srcdirs] exists [missing ()] is called
>   * which might give a warning or error.
>   *
> + * Note that the call may succeed whithout copying any file at all. This
> may
> + * happen when the source subdirectory exists but is empty or when
> [filter]
> + * function is too strict to allow any of the files.
> + *
>   * Returns list of copied files.
>   *)
> -and copy_from_virtio_win g inspect srcdir destdir filter missing > +and
copy_from_virtio_win g inspect srcdirs destdir filter missing >    let ret =
ref [] in
>    if is_directory virtio_win then (
>      debug "windows: copy_from_virtio_win: guest tools source
directory
> %s" virtio_win;
>
> -    let dir = virtio_win // srcdir in
> -    if not (is_directory dir) then missing ()
> -    else (
> +    try
> +      let srcdirs = List.map ((//) virtio_win) srcdirs in
> +      let dir = List.find is_directory srcdirs in
>        let cmd = sprintf "cd %s && find -L -type f"
(quote dir) in
>        let paths = external_command cmd in
>        List.iter (
> @@ -328,7 +337,8 @@ and copy_from_virtio_win g inspect srcdir destdir
> filter missing >              List.push_front target_name ret
>            )
>        ) paths
> -    )
> +    with Not_found ->
> +      missing()
>    )
>    else if is_regular_file virtio_win then (
>      debug "windows: copy_from_virtio_win: guest tools source ISO
%s"
> virtio_win;
> @@ -339,9 +349,9 @@ and copy_from_virtio_win g inspect srcdir destdir
> filter missing >        g2#launch ();
>        let vio_root = "/" in
>        g2#mount_ro "/dev/sda" vio_root;
> -      let srcdir = vio_root ^ "/" ^ srcdir in
> -      if not (g2#is_dir srcdir) then missing ()
> -      else (
> +      let srcdirs = List.map ((//) vio_root) srcdirs in
> +      try
> +        let srcdir = List.find g2#is_dir srcdirs in
>          let paths = g2#find srcdir in
>          Array.iter (
>            fun path ->
> @@ -357,7 +367,8 @@ and copy_from_virtio_win g inspect srcdir destdir
> filter missing >                List.push_front target_name ret
>              )
>          ) paths;
> -      );
> +      with Not_found ->
> +        missing();
>        g2#close()
>      with Guestfs.Error msg ->
>        error (f_"%s: cannot open virtio-win ISO file: %s")
virtio_win msg
> --
> 2.20.1
>
>

On Sat, Jan 26, 2019 at 01:19:58PM +0100, Tomáš Golembiovský
wrote:
> The debug message was slightly changed too to better match the similar
> message for ISO case. It refers to the root directory instead of the
> specific subdirectory inside guest tools.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  v2v/windows_virtio.ml | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index 92bf3ec60..94c4774b7 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -308,10 +308,10 @@ and copy_drivers g inspect driverdir >  and
copy_from_virtio_win g inspect srcdir destdir filter missing >    let ret =
ref [] in
>    if is_directory virtio_win then (
> -    let dir = virtio_win // srcdir in
> -    debug "windows: copy_from_virtio_win: guest tools source
directory %s" dir;
> +    debug "windows: copy_from_virtio_win: guest tools source
directory %s" virtio_win;
Be good to put "virtio_win" on the next line.
> -    if not (is_directory srcdir) then missing ()
> +    let dir = virtio_win // srcdir in
> +    if not (is_directory dir) then missing ()
Did this code actually work before?  It seems to be checking if
literally srcdir (as a directory on the host) exists.

But yes, looks good apart from the overlong line.

Rich.
>      else (
>        let cmd = sprintf "cd %s && find -L -type f"
(quote dir) in
>        let paths = external_command cmd in
> -- 
> 2.20.1
> 
> _______________________________________________
> 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 Sat, Jan 26, 2019 at 01:19:59PM +0100, Tomáš Golembiovský
wrote:
> Allow multiple alternative directory names for distributions (or
> distribution familiy) when installing Linux guest tools packages.
> Original naming required that there is a separate directory for every
> version of a distribution (e.g. fc28, fc29, ...). This is inconvenient
> when users want to keep just a single version of the package for the
> distribution.
> 
> For each distribution one can have either a common directory (e.g.
> fedora) or a versioned directory (fedora28). This can also be combined.
> I.e. one can have both `fedora` and `fedora28` in which case `fedora28`
> will be used when converting Fedora 28 guest wheres `fedora` will be
> used when converting guests with any other Fedora version.
> 
> To have better names for unversioned directories the original names
> were changed this way:
> 
>     fc -> fedora
>     el -> rhel
>     lp -> suse
> 
> The original directory names are kept for backward compatibility and are
> aliased to new names as described below. When both new and old name are
> present on file system the new name takes precedence.
> 
>     fc28 -> fedora
>     el6 -> rhel6
>     el7 -> rhel7
>     lp151 -> suse
Can we use an actual existing scheme like libosinfo ID?
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  v2v/windows_virtio.ml | 65 +++++++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index 94c4774b7..cc33d9502 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -186,14 +186,18 @@ let rec install_drivers ((g, _) as reg) inspect rcaps
>  and install_linux_tools g inspect >    let os >      match
inspect.i_distro with
> -    | "fedora" -> Some "fc28"
> +    | "fedora" -> Some [
> +      (sprintf "fedora%d" inspect.i_major_version);
"fedora"; "fc28"]
>      | "rhel" | "centos" | "scientificlinux"
| "redhat-based"
> -    | "oraclelinux" ->
> -      (match inspect.i_major_version with
> -       | 6 -> Some "el6"
> -       | 7 -> Some "el7"
> -       | _ -> None)
> -    | "sles" | "suse-based" | "opensuse"
-> Some "lp151"
> +    | "oraclelinux" -> Some (
> +      [(sprintf "rhel%d" inspect.i_major_version)]
As a general rule in functional languages function application
[ie. calling a function] binds tightest.  So:

  f 1 + 2    means:    (f 1) + 2

You don't need the parentheses around the sprintf statement above or
later in the patch.
> +      @ (match inspect.i_major_version with
> +       | 6 -> ["el6"]
> +       | 7 -> ["el7"]
> +       | _ -> [])
> +      @ ["rhel"])
You could also rewrite the whole awkward list building non-
functionally.  It would end up like this which seems a whole lot more
readable to me:

  let r = ref [] in
  List.push_back r (sprintf "rhel%d" inspect.i_major_version);
  List.push_back r (match ...);
  List.push_back r "rhel";
  Some r
> +    | "sles" | "suse-based" | "opensuse"
-> Some [
> +      (sprintf "fedora%d" inspect.i_major_version);
"suse"; "lp151"]
But this is OK, except drop the extra parens around sprintf.
> +    with Not_found ->
> +      missing()
Needs a space between missing and ‘()’ (and the same later on).

I wonder if this code would be simpler if we started to use the
‘return’ statement:

 
https://github.com/libguestfs/libguestfs/blob/b26bd778b58b78eb59503413fd5b41ac23725690/common/mlstdutils/std_utils.mli#L340-L352

Anyway, looks reasonable.  I keep thinking it needs a test though. 

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Saturday, 26 January 2019 13:19:59 CET Tomáš Golembiovský
wrote:
> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index 94c4774b7..cc33d9502 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -186,14 +186,18 @@ let rec install_drivers ((g, _) as reg) inspect rcaps
>  and install_linux_tools g inspect >    let os >      match
inspect.i_distro with
> -    | "fedora" -> Some "fc28"
> +    | "fedora" -> Some [
> +      (sprintf "fedora%d" inspect.i_major_version);
"fedora"; "fc28"]
>      | "rhel" | "centos" | "scientificlinux"
| "redhat-based"
> -    | "oraclelinux" ->
> -      (match inspect.i_major_version with
> -       | 6 -> Some "el6"
> -       | 7 -> Some "el7"
> -       | _ -> None)
> -    | "sles" | "suse-based" | "opensuse"
-> Some "lp151"
> +    | "oraclelinux" -> Some (
> +      [(sprintf "rhel%d" inspect.i_major_version)]
> +      @ (match inspect.i_major_version with
> +       | 6 -> ["el6"]
> +       | 7 -> ["el7"]
> +       | _ -> [])
> +      @ ["rhel"])
> +    | "sles" | "suse-based" | "opensuse"
-> Some [
> +      (sprintf "fedora%d" inspect.i_major_version);
"suse"; "lp151"]
"fedoraN" for SUSE?
>      | _ -> None in
>  
>    match os with
The code continues as:

  match os with
  | None ->
      warning (f_"don't know how to install guest tools on %s-%d")
        inspect.i_distro inspect.i_major_version

You need to extend it so "Some []" errors out like None does.  It
should not happen, so it is mostly a safety check.

Another option could be to switch from string list option to
string list, with an empty value to indicate no known directories for
a distro.
 
> @@ -201,15 +205,15 @@ and install_linux_tools g inspect >        warning
(f_"don't know how to install guest tools on %s-%d")
>          inspect.i_distro inspect.i_major_version
>    | Some os ->
os -> oses (since it is no more just one).
> + * Note that the call may succeed whithout copying any file at all. This
may
> + * happen when the source subdirectory exists but is empty or when
[filter]
> + * function is too strict to allow any of the files.
Not sure why copy_from_virtio_win should allow an empty list as srcdirs.
IMHO it seems better to have it error out on an empty list.
> -      let srcdir = vio_root ^ "/" ^ srcdir in
> -      if not (g2#is_dir srcdir) then missing ()
> -      else (
> +      let srcdirs = List.map ((//) vio_root) srcdirs in
Note that (//) in this case it is not correct: (//) concatenates using
the path separator of the platform virt-v2v is built on, while these
paths are appliance paths (so / is always assumed).

-- 
Pino Toscano