Re: [Qemu-devel] [PATCH 03/11] savevm: update bdrv_snapshot_find() to find snapshot by id or name

[Eric Blake <eblake@redhat.com>]

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

On 04/16/2013 10:05 AM, Pavel Hrdina wrote:
> 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.

Fair enough, even if it is a bit of churn on the signature and callers.

> 
> We only need to know if that snapshot exists or not. We don't care
> about any error message. If snapshot exists it returns 1 otherwise
> it returns 0.

If you are only returning a yes/no result, bool is better than int.

> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  savevm.c | 73 ++++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 23 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 6af84fd..96a2340 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2197,25 +2197,55 @@ out:
>  }
>  
>  static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,

Return bool...

> -                              const char *name)
> +                              const char *name, const char *id, bool old_match)
>  {
>      QEMUSnapshotInfo *sn_tab, *sn;
> -    int nb_sns, i, ret;
> +    int nb_sns, i, found = 0;

... and initialize 'bool found = false'.

> +
> +    if (!name && !id) {
> +        return found;
> +    }

It may be appropriate to do assert(name || id), if you want to guarantee
that all callers supply at least one of the two arguments (and that the
QMP code where both arguments are optional does its own sanity checking
that the user supplied an argument.

>  
> -    ret = -ENOENT;
>      nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns < 0)
> -        return ret;
> +    if (nb_sns < 0) {
> +        return found;

Calling the return value 'found' makes the code read awkwardly when
reporting failure; maybe keeping it named 'ret' is better after all.

> +    }
> +
>      for(i = 0; i < nb_sns; i++) {

As long as you are touching this code, you could insert the space after
'for'.

>          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 = 1;
> +                break;
> +            }
> +        } else if (name) {
> +            /* for compatibility for old bdrv_snapshot_find call
> +             * will be removed */
> +            if (old_match) {
> +                if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) {
> +                    *sn_info = *sn;
> +                    found = 1;
> +                    break;
> +                }
> +            } else {
> +                if (!strcmp(sn->name, name)) {
> +                    *sn_info = *sn;
> +                    found = 1;
> +                    break;
> +                }
> +            }
> +        } else if (id) {
> +            if (!strcmp(sn->id_str, id)) {
> +                *sn_info = *sn;
> +                found = 1;
> +                break;
> +            }

Note that this says that if both name and id are specified, then that
takes priority, no matter what was specified for old_match.

>          }
>      }
> +
>      g_free(sn_tab);
> -    return ret;
> +    return found;
>  }
>  
>  /*
> @@ -2229,8 +2259,8 @@ 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)
> +        if (bdrv_can_snapshot(bs)
> +                && bdrv_snapshot_find(bs, snapshot, name, name, true))

Needless style change.  The old style was sufficient, with:

    if (bdrv_can_snapshot(bs) &&
        bdrv_snapshot_find(bs, snapshot, name, name, true))

Except that bdrv_snapshot_find(bs, snapshot, name, name, anything)
results in a lookup that requires sn->id_str and sn->name be the same
string, which is not what you intended.

You really want to be using bdrv_snapshot_find(bs, snapshot, NULL, name,
true), if you want the old_match parameter to make a difference.

...
> @@ -2474,7 +2501,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];
> @@ -2505,8 +2532,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->name, sn->id_str,
> +                                        true)) {

This was the only conversion that passed both id and name that looked
correct.  All the other conversions to 'name, name, true' need fixing.

-- 
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 --]