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

On Mon, Mar 09, 2020 at 12:07:08PM +0100, Pino Toscano wrote: > 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. Well we can certainly revert the patch if you think this is wrong. Dan, any views on this?