Laszlo Ersek
2022-May-26 08:53 UTC
[Libguestfs] [PATCH virt-v2v] convert: If listing RPM applications fails, rebuild DB and retry
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#c12What 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
Richard W.M. Jones
2022-May-26 09:23 UTC
[Libguestfs] [PATCH virt-v2v] convert: If listing RPM applications fails, rebuild DB and retry
On Thu, May 26, 2022 at 10:53:59AM +0200, Laszlo Ersek wrote:> 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>Sure thing, thanks! So ... libguestfs commit: 488245ed6c0c5db282ec7fed646e8bc00ce0d487 virt-v2v commit with additional commentary referencing libguestfs commit: 31bf5db25bcfd8a9f5a48cc0523abae28861de9a Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW