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ý <[email protected]> > ---
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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
