[Libguestfs] [PATCH 2/2] v2v: allow alternative directories for distributions

[Richard W.M. Jones]

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 at 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/