Re: [Libguestfs] [PATCH] v2v: linux: use NEVR for querying RPM packages (RHBZ#1669395)

On Tue, Jan 29, 2019 at 03:29:44PM +0100, Pino Toscano wrote: > Use NEVR when querying RPM for the list of files of a package, instead > of ENVR. Also, use the epoch only when non-zero, and version of RPM > supports it. > > The approach is basically copied from what supermin does in its RPM > package handler. > --- > v2v/linux.ml | 52 +++++++++++++++++++++++++++++++--------------------- > 1 file changed, 31 insertions(+), 21 deletions(-) > > diff --git a/v2v/linux.ml b/v2v/linux.ml > index 43449157b..4cf498d29 100644 > --- a/v2v/linux.ml > +++ b/v2v/linux.ml > @@ -80,29 +80,39 @@ let file_list_of_package (g : Guestfs.guestfs) inspect app = > > | "rpm" -> > (* Since RPM allows multiple packages installed with the same > - * name, always check the full ENVR here (RHBZ#1161250). > + * name, always check the full NEVR here (RHBZ#1161250). > + * > + * In RPM < 4.11 query commands that use the epoch number in the > + * package name did not work. > + * > + * For example: > + * RHEL 6 (rpm 4.8.0): > + * $ rpm -q tar-2:1.23-11.el6.x86_64 > + * package tar-2:1.23-11.el6.x86_64 is not installed > + * Fedora 20 (rpm 4.11.2): > + * $ rpm -q tar-2:1.26-30.fc20.x86_64 > + * tar-1.26-30.fc20.x86_64 > *) > + let rpm_major, rpm_minor = > + let ver = List.find_map ( > + function > + | { G.app2_name = name; G.app2_version = version } > + when name = "rpm" -> Some version > + | _ -> None > + ) inspect.i_apps in This has the nasty side effect of Not_found exception escaping if for some reason we can't find the rpm version. I think you should catch that case and assume old RPM.

> + match String.nsplit "." ver with > + | [major] -> int_of_string major, 0 > + | major :: minor :: _ -> int_of_string major, int_of_string minor > + | _ -> error (f_"unrecognized RPM version: %s") ver in Because this comes from the guest the version field could contain any old stuff, making this a bit error prone. I think it would also be nice if we didn't error out in the last case. Something like this seems better to me: let re_rpm_version = PCRE.compile "(\\d+)\\.(\\d+)" ... let ver = if PCRE.matches re_rpm_version ver then (int_of_string (PCRE.sub 1), int_of_string (PCRE.sub 2)) else (0, 0) in let is_rpm_lt_4_11 = ver < (4, 11) in (We already have this sort of code in daemon/inspect_utils.ml, but we can't reuse it).