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
