Libguestfs - Mar 2020 - Re: [PATCH] lib: Autodetect backing format and specify it explicitly.

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;
+    }
+  }
+
   /* Assemble the qemu-img command line. */
   guestfs_int_cmd_add_arg (cmd, "qemu-img");
   guestfs_int_cmd_add_arg (cmd, "create");
-- 
2.25.0

On Thu, Feb 06, 2020 at 17:20:19 +0000, 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).
I thought about this for a while and I think with blockdev we can
securely and without data loss allow image detection of one layer and
given that it doesn't support backing files (raw) or doesn't have any
backing store specified use it with qemu. With such configuration it
would basically allow previous semantics.

The above rules combined with the fact that we can now ask qemu to
refrain from opening backing files allow us to do it securely.

The necessity to not have backing image is necessary to prevent any
silent corruption of images.

The above error will be reported in cases when libvirt can't introspect
the image, e.g. when on some unknown network storage.

I'll post patches next week hopefully. In such case this patch might not
be necessary.

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?

-- 
Pino Toscano

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org