Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter

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

> Finding snapshot by a name which could also be an id isn't best way
> how to do it. There will be rewrite of savevm, loadvm and delvm to
> improve the behavior of these commands. The savevm and loadvm will
> have their own patch series.
> 
> Now bdrv_snapshot_find takes more parameters. The name parameter will
> be matched only against the name of the snapshot and the same applies
> to id parameter.
> 
> There is one exception. If you set the last parameter, the name parameter
> will be matched against the name or the id of a snapshot. This exception
> is only for backward compatibility for other commands and it will be
> dropped after all commands will be rewritten.
> 
> We only need to know if that snapshot exists or not. We don't care
> about any error message. If snapshot exists it returns TRUE otherwise
> it returns FALSE.
> 
> There is also new Error parameter which will containt error messeage if
> something goes wrong.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>   savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
>   1 file changed, 67 insertions(+), 26 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index ba97c41..1622c55 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2262,26 +2262,66 @@ out:
>       return ret;
>   }
> 
> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> -                              const char *name)
> +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> +                               const char *name, const char *id, Error **errp,
> +                               bool old_match)
  suggest directly drop old_match parameter here, or squash patch 12
into this one, mark the change in commit message, then the logic will
be clearer.
  Personally hope to place parameter *id before *name.

>   {
>       QEMUSnapshotInfo *sn_tab, *sn;
> -    int nb_sns, i, ret;
> +    int nb_sns, i;
> +    bool found = false;
> +
> +    assert(name || id);
> 
> -    ret = -ENOENT;
>       nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns < 0)
> -        return ret;
> -    for(i = 0; i < nb_sns; i++) {
> +    if (nb_sns < 0) {
> +        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> +        return found;
> +    }
> +
> +    if (nb_sns == 0) {
> +        error_setg(errp, "Device has no snapshots");
> +        return found;
> +    }
    suggest not set errp here, which can be used to tip exception, but
having not found one is a normal case.

> +
> +    for (i = 0; i < nb_sns; i++) {
>           sn = &sn_tab[i];
> -        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> -            *sn_info = *sn;
> -            ret = 0;
> -            break;
> +        if (name && id) {
> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                found = true;
> +                break;
> +            }
> +        } else if (name) {
> +            /* for compatibility for old bdrv_snapshot_find call
> +             * will be removed */
> +            if (old_match) {
> +                if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
> +                    *sn_info = *sn;
> +                    found = true;
> +                    break;
> +                }
> +            } else {
> +                if (!strcmp(sn->name, name)) {
> +                    *sn_info = *sn;
> +                    found = true;
> +                    break;
> +                }
> +            }
> +        } else if (id) {
> +            if (!strcmp(sn->id_str, id)) {
> +                *sn_info = *sn;
> +                found = true;
> +                break;
> +            }
>           }
>       }
  suggest change the sequence:

if (name && id) {
    for () {
    }
} else if (name){
    for () {
    }
} else if (id) {
    for () {
    }
}
  slightly faster, and make logic more clear.

> +
> +    if (!found) {
> +        error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
  suggest not to set error, since it is a normal case.

> +    }
> +
>       g_free(sn_tab);
> -    return ret;
> +    return found;
>   }
> 
>   /*
> @@ -2296,7 +2336,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>       bs = NULL;
>       while ((bs = bdrv_next(bs))) {
>           if (bdrv_can_snapshot(bs) &&
> -            bdrv_snapshot_find(bs, snapshot, name) >= 0)
> +            bdrv_snapshot_find(bs, snapshot, name, NULL, NULL, true))
>           {
>               bdrv_snapshot_delete(bs, name, &local_err);
>               if (error_is_set(&local_err)) {
> @@ -2358,8 +2398,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>       sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
> 
>       if (name) {
> -        ret = bdrv_snapshot_find(bs, old_sn, name);
> -        if (ret >= 0) {
> +        if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, true)) {
>               pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
>               pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>           } else {
> @@ -2440,6 +2479,7 @@ int load_vmstate(const char *name)
>       BlockDriverState *bs, *bs_vm_state;
>       QEMUSnapshotInfo sn;
>       QEMUFile *f;
> +    Error *local_err = NULL;
>       int ret;
> 
>       bs_vm_state = bdrv_snapshots();
> @@ -2449,9 +2489,10 @@ int load_vmstate(const char *name)
>       }
> 
>       /* Don't even try to load empty VM states */
> -    ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
> -    if (ret < 0) {
> -        return ret;
> +    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, NULL, &local_err, true)) {
> +        error_report("Snapshot doesn't exist: %s", error_get_pretty(local_err));
> +        error_free(local_err);
> +        return -ENOENT;
>       } else if (sn.vm_state_size == 0) {
>           error_report("This is a disk-only snapshot. Revert to it offline "
>               "using qemu-img.");
> @@ -2473,11 +2514,11 @@ int load_vmstate(const char *name)
>               return -ENOTSUP;
>           }
> 
> -        ret = bdrv_snapshot_find(bs, &sn, name);
> -        if (ret < 0) {
> -            error_report("Device '%s' does not have the requested snapshot '%s'",
> -                           bdrv_get_device_name(bs), name);
> -            return ret;
> +        if (!bdrv_snapshot_find(bs, &sn, name, NULL, &local_err, true)) {
> +            error_report("Snapshot doesn't exist for device '%s': %s",
> +                         bdrv_get_device_name(bs), error_get_pretty(local_err));
> +            error_free(local_err);
> +            return -ENOENT;
>           }
>       }
> 
> @@ -2546,7 +2587,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>   {
>       BlockDriverState *bs, *bs1;
>       QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
> -    int nb_sns, i, ret, available;
> +    int nb_sns, i, available;
>       int total;
>       int *available_snapshots;
>       char buf[256];
> @@ -2577,8 +2618,8 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> 
>           while ((bs1 = bdrv_next(bs1))) {
>               if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> -                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> -                if (ret < 0) {
> +                if (!bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL, NULL,
> +                                        true)) {
>                       available = 0;
>                       break;
>                   }
> 


-- 
Best Regards

Wenchao Xia