[Libguestfs] [PATCH] lib: Autodetect backing format and specify it explicitly.

[Pino Toscano]

On Monday, 9 March 2020 11:40:46 CET Richard W.M. Jones wrote:
> On Mon, Mar 09, 2020 at 11:07:20AM +0100, Pino Toscano wrote:
> > On Thursday, 6 February 2020 18:20:19 CET Richard W.M. Jones wrote:
> > > In the guestfs_disk_create API we have traditionally allowed you to
> > > set backingfile without setting backingformat.  The meaning of this is
> > > to let qemu autodetect the backing format when opening the overlay
> > > disk.
> > > 
> > > However libvirt >= 6.0 refuses to even pass such disks to qemu (see
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1798148).
> > > 
> > > For this reason, move the autodetection earlier and make it explicit.
> > > We now autodetect the format of the backing disk at the time of
> > > creation of the overlay, and set that as the backing format in the
> > > overlay disk itself, allowing libvirt to open the disk later.
> > > ---
> > >  lib/create.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/lib/create.c b/lib/create.c
> > > index e2a59b88d..e30286c39 100644
> > > --- a/lib/create.c
> > > +++ b/lib/create.c
> > > @@ -255,6 +255,7 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size,
> > >                     const struct guestfs_disk_create_argv *optargs)
> > >  {
> > >    const char *backingformat = NULL;
> > > +  CLEANUP_FREE char *backingformat_free = NULL;
> > >    const char *preallocation = NULL;
> > >    const char *compat = NULL;
> > >    int clustersize = -1;
> > > @@ -302,6 +303,18 @@ disk_create_qcow2 (guestfs_h *g, const char *filename, int64_t size,
> > >      }
> > >    }
> > >  
> > > +  /* With libvirt >= 6.0 the backing format must be specified. */
> > > +  if (backingfile != NULL && backingformat == NULL) {
> > > +    backingformat = backingformat_free = guestfs_disk_format (g, backingfile);
> > > +    if (!backingformat)
> > > +      return -1;
> > > +    if (STREQ (backingformat, "unknown")) {
> > > +      error (g, _("could not auto-detect the format of the backing file %s"),
> > > +             backingfile);
> > > +      return -1;
> > > +    }
> > > +  }
> > 
> > I see this patch was pushed, even if it was not reviewed, and in
> > general the problem was more (not totally) on libvirt side.
> > 
> > Was it intentional?
> 
> Yes, I pushed it intentionally.  Peter has a partial fix on the
> libvirt side, but it seems to me they are heading in the direction of
> requiring that the format of backing files in the chain being
> specified.  Since our API specifies that a NULL backing format means
> the format will be autodetected (but not by what), I moved the
> detection to here.

One of the arguments was that using qemu-img to detect the format of
an image was unsafe in any case, and thus libvirt does not use it
(it has own parsing code).

To sum up the situation:
- if using qemu-img is safe (with more or less hardening on top), then
  there is no reason to not do that in libvirt
- if using qemu-img is not safe, then I do not see why libguestfs does
  it while libvirt avoids it
Hence, I think this patch in libguestfs is not correct, no matter which
POV/course of action we choose.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20200309/1fbd72e7/attachment.sig>