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

[Alberto Garcia]

On Fri 04 Sep 2015 04:42:17 PM CEST, Eric Blake <address@hidden> wrote:
>> @@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, 
>> const char *device,
>>                         &snapshot, errp);
>>  }
>>  
>> +void qmp_blockdev_snapshot(const char *device, const char *snapshot,
>
> Is 'node' a better name than 'device' here?

I don't have a strong preference (I was just following Kevin's
suggestion), but 'device' seems to be the most common name for this
parameter. This one can take a node name as well, but it will only
accept an active layer anyway... I can change the name if you prefer.

>> +    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)?

I think you're right, thanks!

>> +SQMP
>> +blockdev-snapshot
>> +-----------------
>> +Since 2.5
>> +
>> +Create a snapshot of a block device. 'device' and 'snapshot' both
>> +refer to existing block devices. The former is the one to generate
>> +the snapshot from, and the latter is the target of the snapshot.
>
> Is there any better terminology?  Maybe:
>
> The act of creating a snapshot installs 'device' as the backing image
> of 'snapshot'; additionally, if 'device' is associated with a block
> device, the block device changes to using 'snapshot' as its new active
> image.

Sounds good.

> Hmm - I wonder if that means we should have an optional boolean
> parameter that allows us to avoid the automatic pivot.  After all,
> with 'blockdev-snapshot-sync', you can specify 'device' and omit
> 'node-name' to update the device's active layer, or you can omit
> 'device' and specify 'node-name' to create another qcow2 file but NOT
> install it as the active layer, regardless of which 'node-name' that
> serves as the starting point. So when 'node-name' is the BDS node that
> 'device' is currently visiting, you have control over whether 'device'
> auto-updates to the new BDS.

What's the use case for that?

Berto