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

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
+      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
+    let is_rpm_lt_4_11 +      rpm_major < 4 || (rpm_major = 4 &&
rpm_minor < 11) in
     let pkg_name -      sprintf "%s-%s-%s" app.G.app2_name
-        app.G.app2_version app.G.app2_release in
-    let pkg_name -      if app.G.app2_epoch > 0_l then (
-        (* RHEL 3/4 'rpm' does not support using the epoch prefix.
-         * (RHBZ#1170685).
-         *)
-        let is_rhel_lt_5 -          match inspect with
-          | { i_type = "linux";
-              i_distro = "rhel" | "centos" |
"scientificlinux" |
-                  "oraclelinux" | "redhat-based";
-              i_major_version = v } when v < 5 -> true
-          | _ -> false in
-        if is_rhel_lt_5 then
-          pkg_name
-        else
-          sprintf "%ld:%s" app.G.app2_epoch pkg_name
-      ) else
-        pkg_name in
+      if is_rpm_lt_4_11 || app.G.app2_epoch = Int32.zero then
+        sprintf "%s-%s-%s" app.G.app2_name app.G.app2_version
+          app.G.app2_release
+      else
+        sprintf "%s-%ld:%s-%s" app.G.app2_name app.G.app2_epoch
+          app.G.app2_version app.G.app2_release in
     let cmd = [| "rpm"; "-ql"; pkg_name |] in
     debug "%s" (String.concat " " (Array.to_list cmd));
     let files = g#command_lines cmd in
-- 
2.20.1

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).

Rich.
> +    let is_rpm_lt_4_11 > +      rpm_major < 4 || (rpm_major = 4
&& rpm_minor < 11) in
>      let pkg_name > -      sprintf "%s-%s-%s" app.G.app2_name
> -        app.G.app2_version app.G.app2_release in
> -    let pkg_name > -      if app.G.app2_epoch > 0_l then (
> -        (* RHEL 3/4 'rpm' does not support using the epoch prefix.
> -         * (RHBZ#1170685).
> -         *)
> -        let is_rhel_lt_5 > -          match inspect with
> -          | { i_type = "linux";
> -              i_distro = "rhel" | "centos" |
"scientificlinux" |
> -                  "oraclelinux" | "redhat-based";
> -              i_major_version = v } when v < 5 -> true
> -          | _ -> false in
> -        if is_rhel_lt_5 then
> -          pkg_name
> -        else
> -          sprintf "%ld:%s" app.G.app2_epoch pkg_name
> -      ) else
> -        pkg_name in
> +      if is_rpm_lt_4_11 || app.G.app2_epoch = Int32.zero then
> +        sprintf "%s-%s-%s" app.G.app2_name app.G.app2_version
> +          app.G.app2_release
> +      else
> +        sprintf "%s-%ld:%s-%s" app.G.app2_name app.G.app2_epoch
> +          app.G.app2_version app.G.app2_release in
>      let cmd = [| "rpm"; "-ql"; pkg_name |] in
>      debug "%s" (String.concat " " (Array.to_list
cmd));
>      let files = g#command_lines cmd in
> -- 
> 2.20.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Wednesday, 30 January 2019 12:02:11 CET Richard W.M. Jones
wrote:
> 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.
More than "can't find the rpm version", the loop looks for rpm in
the
list of installed application.  I can add an error handling, although
I don't think it will happen that an RPM-based distro (detected as such
by the inspection code) will not have the "rpm" package installed...
> > +      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).
I wanted to avoid running a regex every time RPM is queries for the
list of files in a package.  Nevertheless, I took the approach, making
it used only when the epoch is not zero.

-- 
Pino Toscano