Re: [Qemu-devel] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Kevin Wolf]

Am 06.10.2015 um 17:49 hat Alberto Garcia geschrieben:
> On Tue 06 Oct 2015 05:30:07 PM CEST, Kevin Wolf wrote:
> >> -    options = qdict_new();
> >> -    if (has_snapshot_node_name) {
> >> -        qdict_put(options, "node-name",
> >> -                  qstring_from_str(snapshot_node_name));
> >> +        if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
> >> +            error_setg(errp, "New snapshot node name already exists");
> >> +            return;
> >> +        }
> >
> > Preexisting, but shouldn't we use bdrv_lookup_bs() here (because devices
> > and node names share a namespace)?
> 
> I think you're right, good catch!
> 
> >> +    if (state->new_bs->blk != NULL) {
> >> +        error_setg(errp, "The snapshot is already in use by %s",
> >> +                   blk_name(state->new_bs->blk));
> >> +        return;
> >> +    }
> >
> > Is it even possible yet to create a root BDS without a BB?
> 
> It is possible with Max's series, on which mine depends.
> 
>    http://patchwork.ozlabs.org/patch/519375/

Okay. I missed this dependency, it doesn't seem to be very explicit in
the cover letter.

> >> +    if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
> >> +                           errp)) {
> >> +        return;
> >> +    }
> >> +
> >> +    if (state->new_bs->backing_hd != NULL) {
> >> +        error_setg(errp, "The snapshot already has a backing image");
> >>      }
> >
> > The error cases after bdrv_open() should probably bdrv_unref() the
> > node.
> 
> I don't think it's necessary, external_snapshot_abort() already takes
> care of that.

Sorry for the noise, you're right. I was confused by bdrv_reopen()
transactions working differently: There, abort isn't called for the
queue entry that has failed prepare, but here it is.

Kevin