qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alberto Garcia
Subject: Re: [Qemu-devel] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command
Date: Tue, 06 Oct 2015 17:49:59 +0200
User-agent: Notmuch/0.13.2 (http://notmuchmail.org) Emacs/23.2.1 (i486-pc-linux-gnu)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]