Libguestfs - May 2022 - [PATCH virt-v2v] convert: If listing RPM applications fails, rebuild DB and retry

In libguestfs we didn't bother to check the return values from any
librpm calls.  In some cases where possibly the RPM database is
faulty, this caused us to return a zero-length list of installed
applications (but no error indication).  Libguestfs has subsequently
been fixed so now it returns an error if the RPM database is corrupt.

This commit changes virt-v2v behaviour so that if either
guestfs_inspect_list_applications2 returns a zero-length list (ie. old
libguestfs) or it throws an error (new libguestfs) then we attempt to
rebuild the RPM database and retry the operation.  Rebuilding the
database can recover from some but not all RPM DB corruption.

See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2089623#c12
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2089623
Reported-by: Xiaodai Wang
Reported-by: Ming Xie
---
 convert/inspect_source.ml | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml
index 1736009629..16058de644 100644
--- a/convert/inspect_source.ml
+++ b/convert/inspect_source.ml
@@ -34,6 +34,7 @@ let rec inspect_source root_choice g   
reject_if_not_installed_image g root;
 
   let typ = g#inspect_get_type root in
+  let package_format = g#inspect_get_package_format root in
 
   (* Mount up the filesystems. *)
   let mps = g#inspect_get_mountpoints root in
@@ -71,7 +72,7 @@ let rec inspect_source root_choice g    ) mps;
 
   (* Get list of applications/packages installed. *)
-  let apps = g#inspect_list_applications2 root in
+  let apps = list_applications g root package_format in
   let apps = Array.to_list apps in
 
   (* A map of app2_name -> application2, for easier lookups.  Note
@@ -106,7 +107,7 @@ let rec inspect_source root_choice g      i_arch =
g#inspect_get_arch root;
     i_major_version = g#inspect_get_major_version root;
     i_minor_version = g#inspect_get_minor_version root;
-    i_package_format = g#inspect_get_package_format root;
+    i_package_format = package_format;
     i_package_management = g#inspect_get_package_management root;
     i_product_name = g#inspect_get_product_name root;
     i_product_variant = g#inspect_get_product_variant root;
@@ -186,6 +187,27 @@ and reject_if_not_installed_image g root    if fmt <>
"installed" then
     error (f_"libguestfs thinks this is not an installed operating system
(it might be, for example, an installer disk or live CD).  If this is wrong, it
is probably a bug in libguestfs.  root=%s fmt=%s") root fmt
 
+(* Wrapper around g#inspect_list_applications2 which, for RPM
+ * guests, on failure tries to rebuild the RPM database before
+ * repeating the operation.
+ *)
+and list_applications g root = function
+  | "rpm" ->
+     (* RPM guest. *)
+     (try
+        let apps = g#inspect_list_applications2 root in
+        if apps = [||] then raise (G.Error "no applications
returned");
+        apps
+      with G.Error msg ->
+        debug "%s" msg;
+        debug "rebuilding RPM database and retrying ...";
+        ignore (g#sh "rpmdb --rebuilddb");
+        g#inspect_list_applications2 root
+     )
+  | _ ->
+     (* Non-RPM guest, just do it. *)
+     g#inspect_list_applications2 root
+
 (* See if this guest could use UEFI to boot.  It should use GPT and
  * it should have an EFI System Partition (ESP).
  *
-- 
2.35.1

On 05/25/22 18:02, Richard W.M. Jones wrote:
> In libguestfs we didn't bother to check the return values from any
> librpm calls.  In some cases where possibly the RPM database is
> faulty, this caused us to return a zero-length list of installed
> applications (but no error indication).  Libguestfs has subsequently
> been fixed so now it returns an error if the RPM database is corrupt.
> 
> This commit changes virt-v2v behaviour so that if either
> guestfs_inspect_list_applications2 returns a zero-length list (ie. old
> libguestfs) or it throws an error (new libguestfs) then we attempt to
> rebuild the RPM database and retry the operation.  Rebuilding the
> database can recover from some but not all RPM DB corruption.
> 
> See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2089623#c12
What an absolutely horrific error mode. Great job debugging it!
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2089623
> Reported-by: Xiaodai Wang
> Reported-by: Ming Xie
> ---
>  convert/inspect_source.ml | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml
> index 1736009629..16058de644 100644
> --- a/convert/inspect_source.ml
> +++ b/convert/inspect_source.ml
> @@ -34,6 +34,7 @@ let rec inspect_source root_choice g >   
reject_if_not_installed_image g root;
>  
>    let typ = g#inspect_get_type root in
> +  let package_format = g#inspect_get_package_format root in
>  
>    (* Mount up the filesystems. *)
>    let mps = g#inspect_get_mountpoints root in
> @@ -71,7 +72,7 @@ let rec inspect_source root_choice g >    ) mps;
>  
>    (* Get list of applications/packages installed. *)
> -  let apps = g#inspect_list_applications2 root in
> +  let apps = list_applications g root package_format in
>    let apps = Array.to_list apps in
>  
>    (* A map of app2_name -> application2, for easier lookups.  Note
> @@ -106,7 +107,7 @@ let rec inspect_source root_choice g >      i_arch =
g#inspect_get_arch root;
>      i_major_version = g#inspect_get_major_version root;
>      i_minor_version = g#inspect_get_minor_version root;
> -    i_package_format = g#inspect_get_package_format root;
> +    i_package_format = package_format;
>      i_package_management = g#inspect_get_package_management root;
>      i_product_name = g#inspect_get_product_name root;
>      i_product_variant = g#inspect_get_product_variant root;
> @@ -186,6 +187,27 @@ and reject_if_not_installed_image g root >    if
fmt <> "installed" then
>      error (f_"libguestfs thinks this is not an installed operating
system (it might be, for example, an installer disk or live CD).  If this is
wrong, it is probably a bug in libguestfs.  root=%s fmt=%s") root fmt
>  
> +(* Wrapper around g#inspect_list_applications2 which, for RPM
> + * guests, on failure tries to rebuild the RPM database before
> + * repeating the operation.
> + *)
> +and list_applications g root = function
> +  | "rpm" ->
> +     (* RPM guest. *)
> +     (try
[*]
> +        let apps = g#inspect_list_applications2 root in
> +        if apps = [||] then raise (G.Error "no applications
returned");
> +        apps
> +      with G.Error msg ->
> +        debug "%s" msg;
> +        debug "rebuilding RPM database and retrying ...";
> +        ignore (g#sh "rpmdb --rebuilddb");
> +        g#inspect_list_applications2 root
> +     )
> +  | _ ->
> +     (* Non-RPM guest, just do it. *)
> +     g#inspect_list_applications2 root
> +
>  (* See if this guest could use UEFI to boot.  It should use GPT and
>   * it should have an EFI System Partition (ESP).
>   *
> 
The commit message explains well why the
"g#inspect_list_applications2"
method call that is at the top of each "match" pattern cannot be
factored out (to a common call just before the "match"). However,
looking at the code, it's not easy to understand.

Can you please:

(1) Commit the libguestfs patch,

(2) insert a comment at [*], saying that either of the two lines just
below may raise an exception, and which one does depends on libguestfs
having the commit <HASH> from point (1)?

With that:

Reviewed-by: Laszlo Ersek <lersek at redhat.com>


Thanks
Laszlo