On 05/26/22 11:23, Richard W.M. Jones wrote: > 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 <[email protected]> > > Sure thing, thanks! > > So ... > > libguestfs commit: 488245ed6c0c5db282ec7fed646e8bc00ce0d487 > > virt-v2v commit with additional commentary referencing libguestfs > commit: 31bf5db25bcfd8a9f5a48cc0523abae28861de9a
Thank you, that's a very good comment in there. Laszlo _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
