Libguestfs - Feb 2019 - Re: [PATCH 2/2] v2v: allow alternative directories for distributions

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

> > + * 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.
> 
This is not what I am trying to say there. What I am trying to say is:
you pass non-empty [srcdirs] and there is a matching directory that
fits that request; if this directory contains no files then the call
itself succeeds (because some directory was found) but no files were
copied.

Let me know if you have tips on how to phrase that better.

    Tomas


-- 
Tom?? Golembiovsk? <tgolembi at redhat.com>

On Wednesday, 6 February 2019 14:11:41 CET Tomáš Golembiovský
wrote:
> > > + * 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.
> > 
> 
> This is not what I am trying to say there.
 
I was not commenting on that phrasing, but on the actual behaviour.
copy_from_virtio_win can be called with srcdirs as empty list, and in
that case it will do nothing: IMHO this situation is not a valid one.

-- 
Pino Toscano