Re: [Libguestfs] [PATCH 2/2] v2v: allow alternative directories for distributions
On Wed, 06 Feb 2019 17:02:57 +0100 Pino Toscano wrote: > On Wednesday, 6 February 2019 14:11:41 CET Tomáš Golembiovský wrote: > > > > + * Note that the call may succeed whithout copying any file at all. > > > > This may > > > > + * happen when the source subdirectory exists but is empty or when > > > > [filter] > > > > + * function is too strict to allow any of the files. > > > > > > Not sure why copy_from_virtio_win should allow an empty list as srcdirs. > > > IMHO it seems better to have it error out on an empty list. > > > > > > > This is not what I am trying to say there. > > I was not commenting on that phrasing, but on the actual behaviour. > copy_from_virtio_win can be called with srcdirs as empty list, and in > that case it will do nothing: IMHO this situation is not a valid one. ok, got it > > -- > Pino Toscano -- Tomáš Golembiovský ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH 2/2] v2v: allow alternative directories for distributions
On Wednesday, 6 February 2019 14:11:41 CET Tomáš Golembiovský wrote: > > > + * Note that the call may succeed whithout copying any file at all. This > > > may > > > + * happen when the source subdirectory exists but is empty or when > > > [filter] > > > + * function is too strict to allow any of the files. > > > > Not sure why copy_from_virtio_win should allow an empty list as srcdirs. > > IMHO it seems better to have it error out on an empty list. > > > > This is not what I am trying to say there. I was not commenting on that phrasing, but on the actual behaviour. copy_from_virtio_win can be called with srcdirs as empty list, and in that case it will do nothing: IMHO this situation is not a valid one. -- Pino Toscano signature.asc Description: This is a digitally signed message part. ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH 2/2] v2v: allow alternative directories for distributions
> > + * Note that the call may succeed whithout copying any file at all. This > > may > > + * happen when the source subdirectory exists but is empty or when [filter] > > + * function is too strict to allow any of the files. > > Not sure why copy_from_virtio_win should allow an empty list as srcdirs. > IMHO it seems better to have it error out on an empty list. > This is not what I am trying to say there. What I am trying to say is: you pass non-empty [srcdirs] and there is a matching directory that fits that request; if this directory contains no files then the call itself succeeds (because some directory was found) but no files were copied. Let me know if you have tips on how to phrase that better. Tomas -- Tomáš Golembiovský ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH 2/2] v2v: allow alternative directories for distributions
On Mon, 28 Jan 2019 17:39:23 + "Richard W.M. Jones" 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ý > > --- > > 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 + 2means:(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ý ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH 2/2] v2v: allow alternative directories for distributions
On Saturday, 26 January 2019 13:19:59 CET Tomáš Golembiovský wrote: > 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)] > + @ (match inspect.i_major_version with > + | 6 -> ["el6"] > + | 7 -> ["el7"] > + | _ -> []) > + @ ["rhel"]) > +| "sles" | "suse-based" | "opensuse" -> Some [ > + (sprintf "fedora%d" inspect.i_major_version); "suse"; "lp151"] "fedoraN" for SUSE? > | _ -> None in > >match os with The code continues as: match os with | None -> warning (f_"don't know how to install guest tools on %s-%d") inspect.i_distro inspect.i_major_version You need to extend it so "Some []" errors out like None does. It should not happen, so it is mostly a safety check. Another option could be to switch from string list option to string list, with an empty value to indicate no known directories for a distro. > @@ -201,15 +205,15 @@ and install_linux_tools g inspect = >warning (f_"don't know how to install guest tools on %s-%d") > inspect.i_distro inspect.i_major_version >| Some os -> os -> oses (since it is no more just one). > + * Note that the call may succeed whithout copying any file at all. This may > + * happen when the source subdirectory exists but is empty or when [filter] > + * function is too strict to allow any of the files. Not sure why copy_from_virtio_win should allow an empty list as srcdirs. IMHO it seems better to have it error out on an empty list. > - let srcdir = vio_root ^ "/" ^ srcdir in > - if not (g2#is_dir srcdir) then missing () > - else ( > + let srcdirs = List.map ((//) vio_root) srcdirs in Note that (//) in this case it is not correct: (//) concatenates using the path separator of the platform virt-v2v is built on, while these paths are appliance paths (so / is always assumed). -- Pino Toscano signature.asc Description: This is a digitally signed message part. ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
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ý > --- > 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 + 2means:(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/ ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH 2/2] v2v: allow alternative directories for distributions
Il giorno sab 26 gen 2019, 13:20 Tomáš Golembiovský ha scritto: > 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 > this will cut centos off 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ý > --- > 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)] > + @ (match inspect.i_major_version with > + | 6 -> ["el6"] > + | 7 -> ["el7"] > + | _ -> []) > + @ ["rhel"]) > +| "sles" | "suse-based" | "opensuse" -> Some [ > + (sprintf "fedora%d" inspect.i_major_version); "suse"; "lp151"] > | _ -> None in > >match os with > @@ -201,15 +205,15 @@ and install_linux_tools g inspect = >warning (f_"don't know how to install guest tools on %s-%d") > inspect.i_distro inspect.i_major_version >| Some os -> > - let src_path = "linux" // os in > + let src_paths = List.map ((//) "linux") os in >let dst_path = "/var/tmp" in > - debug "locating packages in %s" src_path; > + debug "locating packages in: %s" (String.concat ", " src_paths); >let packages = > -copy_from_virtio_win g inspect src_path dst_path > +copy_from_virtio_win g inspect src_paths dst_path > (fun _ _ -> true) > (fun () -> > - warning (f_"guest tools directory ‘%s’ is > missing from the virtio-win directory or ISO.\n\nGuest tools are only > provided in the RHV Guest Tools ISO, so this can happen if you are using > the version of virtio-win which contains just the virtio drivers. In this > case only virtio drivers can be installed in the guest, and installation of > Guest Tools will be skipped.") > - src_path) in > + warning (f_"none of the guest tools > directories ‘[%s]’ was found on the virtio-win directory or ISO.\n\nGuest > tools are only provided in the oVirt/RHV Guest Tools ISO, so this can > happen if you are using the version of virtio-win which contains just the > virtio drivers. In this case only virtio drivers can be installed in the > guest, and installation of Guest Tools will be skipped.") > + (String.concat ", " src_paths)) in >debug "done copying %d files" (List.length packages); >let packages = List.map ((//) dst_path) packages in >try > @@ -290,29 +294,34 @@ and ddb_regedits inspect drv_name drv_pciid = > * been copied. > *) > and copy_drivers g inspect driverdir = > - [] <> copy_from_virtio_win g inspect "/" driverdir > + [] <> copy_from_virtio_win g inspect ["/"] driverdir > virtio_iso_path_matches_guest_os > (fun () -> >error (f_"root directory ‘/’ is missing from the virtio-win > directory or ISO.\n\nThis should not happen and may indicate that > virtio-win or virt-v2v is broken in some way. Please report this as a bug > with a full debug log.")) > > -(* Copy all files from virtio_win directory/ISO located in [srcdir] > - *
Re: [Libguestfs] [PATCH 2/2] v2v: allow alternative directories for distributions
On Sat, 26 Jan 2019 19:42:15 +0100 Sandro Bonazzola wrote: > Il giorno sab 26 gen 2019, 13:20 Tomáš Golembiovský > ha scritto: > > > 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 > > > > > this will cut centos off > > Nope, `rhel` directory is used also for CentOS (as well as oraclelinux and scientific linux). > > > lp -> suse Similarly here it's SLES as well as Leap. > > > > 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ý > > --- > > v2v/windows_virtio.ml | 65 +-- > > 1 file changed, 38 insertions(+), 27 deletions(-) ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
