Re: [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This patch adds a parameter to tell whether return valid snapshots
> for whole VM only.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
>  include/block/qapi.h |    1 +
>  qemu-img.c           |    3 ++-
>  3 files changed, 40 insertions(+), 3 deletions(-)
> 

> + * @sn: snapshot info need to be checked.

s/need //

> + * return 0 if valid, it means load_vmstate() for it could succeed.

Reads awkwardly; how about:

it means load_vmstate() could load that snapshot.

> + */
> +static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
> +{
> +    BlockDriverState *bs1 = NULL;
> +    QEMUSnapshotInfo s, *sn_info = &s;
> +    int ret = 0;
> +
> +    /* Check logic is connected with load_vmstate():
> +       Only check the devices that can snapshot, other devices that can't
> +       take snapshot, for example, readonly ones, will be ignored in
> +       load_vmstate(). */
> +    while ((bs1 = bdrv_next(bs1))) {
> +        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> +            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
> +            if (ret < 0) {
> +                ret = -1;

This function only returns 0 or -1...

> +/* return 0 on success, and @p_list will be set only on success. If
> +   @vm_snapshot is true, only the snapshot valid for whole vm will be got. */

grammar

If @vm_snapshot is true, limit the results to snapshots valid for the
whole vm.

> -
> +        if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {

...yet you are only ever calling it in a boolean context.  Would it be
smarter to have the function return bool (true for valid vm snapshot,
false if the image snapshot is not usable as a vm snapshot)?

Also, it might be nicer to name it snapshot_valid_for_vm, as in:

if (vm_snapshot && !snapshot_valid_for_vm(...)) {
    continue;
}

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]