Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command

[Kevin Wolf]

Am 07.09.2015 um 13:31 hat Alberto Garcia geschrieben:
> On Fri 04 Sep 2015 04:42:17 PM CEST, Eric Blake <address@hidden> wrote:
> >> +    if (snapshot_ref) {
> >> +        if (!bdrv_lookup_bs(snapshot_ref, snapshot_ref, &local_err)) {
> >>              error_propagate(errp, local_err);
> >>              return;
> >>          }
> >>      }
> >
> > Shouldn't you also check that snapshot_ref does not currently have a
> > backing BDS (as it is the act of creating the snapshot that sets the
> > current device as the backing of the snapshot_ref BDS before altering
> > the BB to point to snapshot_ref as its new BDS)?
> 
> Wait, I actually think that it should have a backing BDS. This new
> command is roughly equivalent to the 'existing' mode of the
> 'blockdev-snapshot-sync' command, so the new image must be created
> externally in both cases having the current image as its backing file.
> 
> The difference is that 'blockdev-snapshot-sync' will open the new image
> and not its backing chain (using the BDRV_O_NO_BACKING flag), but in
> 'blockdev-snapshot' the image must be opened previously with
> 'blockdev-add', which will open the whole chain.
> 
> So I think that we should expect a backing image and we have to unref it
> during the 'commit' part of the transaction (before bdrv_append()).

This means that the images in the backing chain are opened twice before
doing the bdrv_append(). This definitely includes the active layer as
one example of an illegal setup, and with block jobs running on the
backing files it might involve even more writable image files.

I think Eric's approach is better, where we check the required
conditions instead of adding magic to somehow transfer a questionable
state into what we want.

Of course, this means that we need to provide a way to actually fulfill
the right conditions, and you might be right that it doesn't exist yet.
Could the same effect as BDRV_O_NO_BACKING possibly be achieved with
blockdev-add with something like '"backing": {}'?

We might actually have the same problem in more places: In optional
fields, we need to distinguish "use the default" from "I don't want
any". Especially if we want to support reopen in QMP at some point, we
need to figure this out (default is leaving options unchanged, but
unsetting must be possible).

Kevin