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

[Richard W.M. Jones]

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