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

[Pino Toscano]

On Friday, 8 February 2019 11:44:42 CET 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
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi at redhat.com>
> ---

Mostly LGTM -- few notes below.

> diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
> index a2b59d1ec..9ef4904be 100644
> --- a/v2v/windows_virtio.ml
> +++ b/v2v/windows_virtio.ml
> @@ -184,32 +184,38 @@ let rec install_drivers ((g, _) as reg) inspect rcaps =
>    )
>  
>  and install_linux_tools g inspect =
> -  let os =
> +  let oses =
>      match inspect.i_distro with
> -    | "fedora" -> Some "fc28"
> +    | "fedora" -> [
> +      sprintf "fedora%d" inspect.i_major_version; "fedora"; "fc28"]

The indentation here is funky... I'd put the whole array in its own
line.

>      | "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"
> -    | _ -> None in
> -
> -  match os with
> -  | None ->
> +      let r = ref [] in
> +      List.push_back r (sprintf "rhel%d" inspect.i_major_version);
> +      List.push_back_list r (match inspect.i_major_version with
> +        | 6 -> ["el6"]
> +        | 7 -> ["el7"]

Why not just 'sprintf "el%d" inspect.i_major_version'?

> +        | _ -> []);
> +      List.push_back r "rhel";
> +      !r
> +    | "sles" | "suse-based" | "opensuse" -> [
> +      sprintf "suse%d" inspect.i_major_version; "suse"; "lp151"]

Funky indentation here too.

> @@ -340,9 +354,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

Paths in the appliance are always with '/', so you cannot use (^) to
join appliance paths (as (^) expands to the path separator of the OS
where virt-v2v runs).
In practice it is not a problem, since so far libguestfs runs on Unix
platforms only.  However, it would be nice to get it correct, so there
is less potential confusion between host paths, and appliance paths.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190321/0034f540/attachment.sig>