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

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(a)redhat.com&gt; > --- > 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/b26bd778b58b78eb59503413fd5... 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/