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/
>> + 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.
Thanks for reviewing!
Berto
