On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This interface now return valid internal snapshots for whole vm.

s/now return/returns/

> 
> 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'.

>  ##
> +# @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/


>  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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org