Re: [Qemu-devel] [PATCH V8 10/20] qmp: add interface query-snapshots

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-3-9 7:30, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This interface now return valid internal snapshots for whole vm.
>
> s/now return/returns/
>
   OK.

>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qapi.c     |   22 +++++++++++++++++++++
>>   qapi-schema.json |   14 +++++++++++++
>>   qmp-commands.hx  |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 91 insertions(+), 0 deletions(-)
>>
>
>> +
>> +SnapshotInfoList *qmp_query_snapshots(Error **errp)
>> +{
>> +    BlockDriverState *bs;
>> +    SnapshotInfoList *list = NULL;
>> +    int ret;
>> +
>> +    /* internal snapshot for whole vm */
>> +    bs = bdrv_snapshots();
>> +    if (!bs) {
>> +        error_setg(errp, "No available block device supports snapshots\n");
>> +        return NULL;
>> +    }
>> +
>
> list is NULL here,
>
>> +    ret = bdrv_query_snapshot_info_list(bs, &list, true, errp);
>> +    if (ret < 0) {
>> +        qapi_free_SnapshotInfoList(list);
>
> and you documented that bdrv_query_snapshot_info_list leaves list
> untouched on error, so why do you have to clean it up?
>
>> +        list = NULL;
>> +    }
>> +    return list;
>
> In fact, you could write this as:
>
>      bdrv_query_snapshot_info_list(bs, &list, true, errp);
>      return list;
>
> without needing 'ret'.
>
   Yep, you are right.

>>   ##
>> +# @query-snapshots:
>> +#
>> +# Get a list of internal snapshots for whole virtual machine, only valid
>
> s/for/for the/; s/machine, only/machine. Only/
>
>> +# internal snapshot will be returned, inconsistent ones will be ignored
>
> s/snapshot/snapshots/
>
   OK.

>
>>   SQMP
>> +query-snapshots
>> +-----------
>
> Common practice is to make the divider line match the line length of the
> line above (you were short by '----')
>
>> +
>> +Show the internal consistent snapshot information
>> +
>> +Each snapshot is represented by a json-object. The returned value
>> +is a json-array of all snapshots
>> +
>> +Each json-object contain the following:
>> +
>> +- "id": unique snapshot id (json-string)
>> +- "name": internal snapshot name (json-string)
>> +- "vm-state-size": size of the VM state in bytes (json-int)
>> +- "date-sec": UTC date of the snapshot in seconds (json-int)
>> +- "date-nsec": fractional part in nano seconds to be used with
>
> s/nano seconds/nanoseconds/
>
>> +               date-sec(json-int)
>> +- "vm-clock-sec": VM clock relative to boot in seconds (json-int)
>> +- "vm-clock-nsec": fractional part in nano seconds to be used with
>
> and again
>
OK.


-- 
Best Regards

Wenchao Xia