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

[Kevin Wolf <kwolf@redhat.com>]

Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> 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 1 otherwise
> it returns 0.

Sounds like it should be true/false rather than 1/0.

Also, isn't there a difference between "there is no snapshot foo" and "we
couldn't find out if there is a snapshot foo because there was an
error"?

> 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,
> -                              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;
> +
> +    if (!name && !id) {
> +        return found;
> +    }

Why not just an explicit return 0? Or actually, while you touch the
prototype you could make the function return a bool and return false.

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

Same here.

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

You can add a space after 'for' while touching the function.

>          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;
> +            }
>          }
>      }
> +
>      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))

It would only retain the old behaviour if you specified NULL for id.
Now _both_ name and id must match, which will happen almost never.

How did you test this patch? Sounds like something that a qemu-iotests
case using the Python QMP methods could easily test.

>          {
>              bdrv_snapshot_delete(bs, name, &local_err);
>              if (error_is_set(&local_err)) {
> @@ -2289,8 +2319,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, name, true)) {

Same problem.

>              pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
>              pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>          } else {
> @@ -2380,9 +2409,8 @@ 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, name, true)) {

And again.

> +        return -ENOENT;

We're losing the real error message here. The old code said exactly what
happened, the new one always claims that the snapshot doesn't exist even
in case of I/O errors or anything.

>      } else if (sn.vm_state_size == 0) {
>          error_report("This is a disk-only snapshot. Revert to it offline "
>              "using qemu-img.");
> @@ -2404,11 +2432,10 @@ int load_vmstate(const char *name)
>              return -ENOTSUP;
>          }
>  
> -        ret = bdrv_snapshot_find(bs, &sn, name);
> -        if (ret < 0) {
> +        if (!bdrv_snapshot_find(bs, &sn, name, name, true)) {
>              error_report("Device '%s' does not have the requested snapshot '%s'",
>                             bdrv_get_device_name(bs), name);
> -            return ret;
> +            return -ENOENT;
>          }
>      }
>  
> @@ -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)) {

Is it intention that both id and name must match here? I suspect that no
because you set old_match == true, which is however ignored because both
id and name are set.

>                      available = 0;
>                      break;
>                  }

Kevin