Libguestfs - Oct 2015 - [PATCH 0/4] rpm: Choose providers better (RHBZ#1266918).

Fix for
https://bugzilla.redhat.com/show_bug.cgi?id=1266918

---
 src/librpm.ml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/librpm.ml b/src/librpm.ml
index dc80286..4eeba77 100644
--- a/src/librpm.ml
+++ b/src/librpm.ml
@@ -16,7 +16,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  *)
 
-external rpm_is_available : unit -> bool =
"supermin_rpm_is_available"
+external rpm_is_available : unit -> bool =
"supermin_rpm_is_available" "noalloc"
 
 external rpm_version : unit -> string = "supermin_rpm_version"
 external rpm_vercmp : string -> string -> int =
"supermin_rpm_vercmp" "noalloc"
-- 
2.5.0

---
 src/librpm.mli | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/librpm.mli b/src/librpm.mli
index da73911..5229be6 100644
--- a/src/librpm.mli
+++ b/src/librpm.mli
@@ -16,17 +16,28 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  *)
 
+(** Wrappers around [librpm] functions. *)
+
 val rpm_is_available : unit -> bool
+(** Returns [true] iff librpm is supported.  If this returns [false],
+    then all other functions will abort. *)
 
 val rpm_version : unit -> string
+(** The linked version of librpm. *)
+
 val rpm_vercmp : string -> string -> int
+(** Compare two RPM version strings using RPM version compare rules. *)
 
 type t
+(** The librpm handle. *)
 
 exception Multiple_matches of int
 
 val rpm_open : ?debug:int -> t
+(** Open the librpm (transaction set) handle. *)
 val rpm_close : t -> unit
+(** Explicitly close the handle.  The handle can also be closed by
+    the garbage collector if it becomes unreachable. *)
 
 type rpm_t = {
   name : string;
@@ -44,6 +55,16 @@ type rpmfile_t = {
   | FileConfig
 
 val rpm_installed : t -> string -> rpm_t array
+(** Return the list of packages matching the name
+    (similar to [rpm -q name]). *)
+
 val rpm_pkg_requires : t -> string -> string array
+(** Return the requires of a package (similar to [rpm -qR]). *)
+
 val rpm_pkg_whatprovides : t -> string -> string array
+(** Return what package(s) provide a particular requirement
+    (similar to [rpm -q --whatprovides]). *)
+
 val rpm_pkg_filelist : t -> string -> rpmfile_t array
+(** Return the list of files contained in a package, and attributes of
+    those files (similar to [rpm -ql]). *)
-- 
2.5.0

Because C and OCaml use different implementations of stdio, and
because C stdout is not line-buffered, we can get mixed up debugging
messages.

Partially fix this by using fflush (stdout) after printing C debugging
messages (this doesn't completely fix it because we'd have to do the
same in OCaml code too).

This slows down debugging output, but it's only used when debugging so
we don't care about that.
---
 src/librpm-c.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/librpm-c.c b/src/librpm-c.c
index fc847d6..3bd25a2 100644
--- a/src/librpm-c.c
+++ b/src/librpm-c.c
@@ -202,8 +202,11 @@ supermin_rpm_installed (value rpmv, value pkgv)
     caml_raise_not_found ();
 
   count = rpmdbGetIteratorCount (iter);
-  if (data.debug >= 2)
-    printf ("supermin: rpm: installed: %d occurrences for
'%s'\n", count, String_val (pkgv));
+  if (data.debug >= 2) {
+    printf ("supermin: rpm: installed: %d occurrences for
'%s'\n",
+            count, String_val (pkgv));
+    fflush (stdout);
+  }
 
   rv = caml_alloc (count, 0);
   i = 0;
@@ -287,8 +290,11 @@ supermin_rpm_pkg_requires (value rpmv, value pkgv)
     caml_raise_not_found ();
 
   count = rpmdbGetIteratorCount (iter);
-  if (data.debug >= 2)
-    printf ("supermin: rpm: pkg_requires: %d occurrences for
'%s'\n", count, String_val (pkgv));
+  if (data.debug >= 2) {
+    printf ("supermin: rpm: pkg_requires: %d occurrences for
'%s'\n",
+            count, String_val (pkgv));
+    fflush (stdout);
+  }
   if (count != 1)
     librpm_raise_multiple_matches (count);
 
@@ -351,8 +357,11 @@ supermin_rpm_pkg_whatprovides (value rpmv, value pkgv)
     caml_raise_not_found ();
 
   count = rpmdbGetIteratorCount (iter);
-  if (data.debug >= 2)
-    printf ("supermin: rpm: pkg_whatprovides: %d occurrences for
'%s'\n", count, String_val (pkgv));
+  if (data.debug >= 2) {
+    printf ("supermin: rpm: pkg_whatprovides: %d occurrences for
'%s'\n",
+            count, String_val (pkgv));
+    fflush (stdout);
+  }
 
   rv = caml_alloc (count, 0);
   i = 0;
@@ -398,8 +407,11 @@ supermin_rpm_pkg_filelist (value rpmv, value pkgv)
     caml_raise_not_found ();
 
   count = rpmdbGetIteratorCount (iter);
-  if (data.debug >= 2)
-    printf ("supermin: rpm: pkg_filelist: %d occurrences for
'%s'\n", count, String_val (pkgv));
+  if (data.debug >= 2) {
+    printf ("supermin: rpm: pkg_filelist: %d occurrences for
'%s'\n",
+            count, String_val (pkgv));
+    fflush (stdout);
+  }
   if (count != 1)
     librpm_raise_multiple_matches (count);
 
-- 
2.5.0

In the referenced bug, a customer had installed a web browser called
'palemoon'.  The RPM of this web browser provides and requires various
core libraries, such as:

  Provides: libnss3.so()(64bit) # normally provided by 'nss'
  Requires: libxul.so()(64bit)  # normally provided by 'firefox'

Our previous algorithm -- inherited from the days when we used to run
'rpm' commands -- takes every provider of a particular requirement and
adds it to a big list, so if any other package requires
'libnss3.so()(64bit)', then both 'nss' and 'palemoon'
are added to the
package list.

Yum used to handle this differently - it used to only pick the package
with the shortest name.  Later on, before yum was retired, it had a
more complex decision algorithm described here:
http://yum.baseurl.org/wiki/CompareProviders

This change makes supermin use the shortest name algorithm, so in the
case above, it always picks 'nss' over 'palemoon'.

There is a second possible problem which is not fixed by the current
patch set: If a package both provides and requires the same
dependency, we should ignore that dependency.  For example, 'palemoon'
both Provides and Requires 'libxul.so()(64bit)', so if
'palemoon' is
pulled in, then 'firefox' would not be an additional requirement.
Because we store all the packages in a big list, we lose track of
where a dependency originates from, so it is not easy to implement
this second change.
---
 src/rpm.ml | 55 +++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/src/rpm.ml b/src/rpm.ml
index 4d31472..d3ab7da 100644
--- a/src/rpm.ml
+++ b/src/rpm.ml
@@ -210,8 +210,42 @@ let rpm_package_name pkg  let
rpm_get_package_database_mtime ()    (lstat
"/var/lib/rpm/Packages").st_mtime
 
-(* Memo of resolved provides. *)
-let rpm_providers = Hashtbl.create 13
+(* Return the best provider of a particular RPM requirement.
+ *
+ * There may be multiple, or no providers.  In case there are multiple,
+ * choose the one with the shortest name (as yum used to).
+ *
+ * We could do better here: http://yum.baseurl.org/wiki/CompareProviders
+ *)
+let provider +  (* Memo of resolved provides. *)
+  let rpm_providers = Hashtbl.create 13 in
+  fun req ->
+    try Hashtbl.find rpm_providers req
+    with Not_found ->
+      try
+        (* 'providers' here is an array of just package names. *)
+        let providers = rpm_pkg_whatprovides (get_rpm ()) req in
+        let providers = Array.to_list providers in
+        (* --whatprovides will return duplicate identical packages, so: *)
+        let providers = sort_uniq providers in
+        match providers with
+        | [] -> None
+        | [name] -> Some name
+        | names ->
+           if !settings.debug >= 2 then
+             printf "supermin: rpm: multiple providers: requirement %s:
providers: %s\n"
+                    req (String.concat " " names);
+           let cmp name1 name2 +             let len1 = String.length name1 and
len2 = String.length name2 in
+             if len1 <> len2 then compare len1 len2
+             else compare name1 name2 in
+           let names = List.sort cmp names in
+           let best_name = List.hd names in
+           if !settings.debug >= 2 then
+             printf "supermin: rpm: multiple providers: picked %s\n"
best_name;
+           Some best_name
+      with Not_found -> None
 
 let rpm_get_all_requires pkgs    let get pkg @@ -226,20 +260,9 @@ let
rpm_get_all_requires pkgs                rpm_pkg_requires (get_rpm ())
(rpm_package_to_string pkg) in
     let pkgs' = Array.fold_left (
       fun set x ->
-        try
-          let provides -            try Hashtbl.find rpm_providers x
-            with Not_found ->
-              let p = rpm_pkg_whatprovides (get_rpm ()) x in
-              Hashtbl.add rpm_providers x p;
-              p in
-          Array.fold_left (
-            fun newset p ->
-              match rpm_package_of_string p with
-                | None -> newset
-                | Some x -> StringSet.add p newset
-          ) set provides
-        with Not_found -> set
+        match provider x with
+        | None -> set
+        | Some p -> StringSet.add p set
     ) StringSet.empty reqs in
     pkgs'
   in
-- 
2.5.0

On Tuesday 13 October 2015 13:54:27 Richard W.M. Jones
wrote:
> Fix for
> https://bugzilla.redhat.com/show_bug.cgi?id=1266918
Patches #1, #2, and #3 LGTM.
Left few notes for patch #4.

Thanks,
-- 
Pino Toscano

On Tuesday 13 October 2015 13:54:31 Richard W.M. Jones
wrote:
> In the referenced bug, a customer had installed a web browser called
> 'palemoon'.  The RPM of this web browser provides and requires
various
> core libraries, such as:
> 
>   Provides: libnss3.so()(64bit) # normally provided by 'nss'
>   Requires: libxul.so()(64bit)  # normally provided by 'firefox'
> 
> Our previous algorithm -- inherited from the days when we used to run
> 'rpm' commands -- takes every provider of a particular requirement
and
> adds it to a big list, so if any other package requires
> 'libnss3.so()(64bit)', then both 'nss' and
'palemoon' are added to the
> package list.
> 
> Yum used to handle this differently - it used to only pick the package
> with the shortest name.  Later on, before yum was retired, it had a
> more complex decision algorithm described here:
> http://yum.baseurl.org/wiki/CompareProviders
> 
> This change makes supermin use the shortest name algorithm, so in the
> case above, it always picks 'nss' over 'palemoon'.
> 
> There is a second possible problem which is not fixed by the current
> patch set: If a package both provides and requires the same
> dependency, we should ignore that dependency.  For example,
'palemoon'
> both Provides and Requires 'libxul.so()(64bit)', so if
'palemoon' is
> pulled in, then 'firefox' would not be an additional requirement.
> Because we store all the packages in a big list, we lose track of
> where a dependency originates from, so it is not easy to implement
> this second change.
> ---
>  src/rpm.ml | 55 +++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/src/rpm.ml b/src/rpm.ml
> index 4d31472..d3ab7da 100644
> --- a/src/rpm.ml
> +++ b/src/rpm.ml
> @@ -210,8 +210,42 @@ let rpm_package_name pkg >  let
rpm_get_package_database_mtime () >    (lstat
"/var/lib/rpm/Packages").st_mtime
>  
> -(* Memo of resolved provides. *)
> -let rpm_providers = Hashtbl.create 13
> +(* Return the best provider of a particular RPM requirement.
> + *
> + * There may be multiple, or no providers.  In case there are multiple,
> + * choose the one with the shortest name (as yum used to).
> + *
> + * We could do better here: http://yum.baseurl.org/wiki/CompareProviders
> + *)
> +let provider > +  (* Memo of resolved provides. *)
> +  let rpm_providers = Hashtbl.create 13 in
> +  fun req ->
> +    try Hashtbl.find rpm_providers req
> +    with Not_found ->
> +      try
> +        (* 'providers' here is an array of just package names. *)
> +        let providers = rpm_pkg_whatprovides (get_rpm ()) req in
> +        let providers = Array.to_list providers in
> +        (* --whatprovides will return duplicate identical packages, so: *)
> +        let providers = sort_uniq providers in
> +        match providers with
> +        | [] -> None
> +        | [name] -> Some name
> +        | names ->
> +           if !settings.debug >= 2 then
> +             printf "supermin: rpm: multiple providers: requirement
%s: providers: %s\n"
> +                    req (String.concat " " names);
> +           let cmp name1 name2 > +             let len1 = String.length
name1 and len2 = String.length name2 in
> +             if len1 <> len2 then compare len1 len2
> +             else compare name1 name2 in
> +           let names = List.sort cmp names in
> +           let best_name = List.hd names in
> +           if !settings.debug >= 2 then
> +             printf "supermin: rpm: multiple providers: picked
%s\n" best_name;
> +           Some best_name
> +      with Not_found -> None
This is nicer indeed, although there are two issues:

- the caching of providers (rpm_providers) is not done, so it will do
  the search every time (and things like "/bin/sh provided by bash"
  appear a lot in packages)

- the old code was checking each provided name was really a package
  (using rpm_package_of_string).  The new code should then pick the
  first one in the list that is actually installed (i.e. with a Some
  result of rpm_package_of_string); the two cases
| +        | [name] -> Some name
| +        | names ->
could be joined together, printing the debug output only if there is
more than one element. I hope List.sort on one element should be a
no-op...

Thanks,
-- 
Pino Toscano