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

[Tomáš Golembiovský]

On Mon, 28 Jan 2019 17:39:23 +0000
"Richard W.M. Jones" <rjones at redhat.com> wrote:

> 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?

I'm not sure how this would work. Note that we are mapping distro
families to those directories and not just single distributions.


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

Rewritten. I don't know if that improved readability but I don't have
any strong preference here.

> 
> > +    | "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. 

I'll look into it.

    Tomas

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


-- 
Tomáš Golembiovský <tgolembi at redhat.com>