Re: [Qemu-devel] [PATCH V5 09/13] block: export function bdrv_find_snapshot()

[Kevin Wolf]

Am 24.01.2013 03:57, schrieb Wenchao Xia:
>   This patch move it from savevm.c to block.c and export it. To make
> it clear about id and name in searching, the API was changed a bit
> to distinguish them. Caller can choose to search by id or name now.
> 
> Signed-off-by: Wenchao Xia <address@hidden>

Please keep code motion and semantic changes separate. One change per patch.

> ---
>  block.c               |   51 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |    2 +
>  savevm.c              |   32 ++++-------------------------
>  3 files changed, 58 insertions(+), 27 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 02c74dc..6631d9a 100644
> --- a/block.c
> +++ b/block.c
> @@ -3395,6 +3395,57 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>      return -ENOTSUP;
>  }
>  
> +/*
> + * Try find an internal snapshot with @id or @name, @id have higher priority
> + * in searching.
> + * @bs block device to search on, must not be NULL.
> + * @sn_info snapshot information to be filled in, must not be NULL.
> + * @id snapshot id to search with, can be NULL.
> + * @name snapshot name to search with, can be NULL.
> + * returns 0 and @sn_info is filled with related information if found,
> + * otherwise it returns negative value, -ENOENT.
> + */
> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> +                       const char *id, const char *name)
> +{
> +    QEMUSnapshotInfo *sn_tab, *sn;
> +    int nb_sns, i, ret;
> +
> +    ret = -ENOENT;
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    if (nb_sns < 0) {
> +        return ret;

Why don't you return the real error?

> +    }
> +
> +    /* search by id */
> +    if (id) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id)) {
> +                *sn_info = *sn;
> +                ret = 0;
> +                goto out;
> +            }
> +        }
> +    }
> +
> +    /* search by name */
> +    if (name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = 0;
> +                goto out;
> +            }
> +        }
> +    }

What's the motivation for duplicating the code instead of keeping it an
strcmp() || strcmp() like in the original place?

> +
> + out:
> +    g_free(sn_tab);
> +    return ret;
> +}
> +
>  /* backing_file can either be relative, or absolute, or a protocol.  If it is
>   * relative, it must be relative to the chain.  So, passing in bs->filename
>   * from a BDS as backing_file should not be done, as that may be relative to
> diff --git a/include/block/block.h b/include/block/block.h
> index 9838c48..1e7f4a1 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -340,6 +340,8 @@ int bdrv_snapshot_list(BlockDriverState *bs,
>  int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>                             const char *snapshot_name);
>  char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> +                       const char *id, const char *name);
>  
>  char *get_human_readable_size(char *buf, int buf_size, int64_t size);
>  int path_is_absolute(const char *path);
> diff --git a/savevm.c b/savevm.c
> index 304d1ef..dccaece 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2029,28 +2029,6 @@ out:
>      return ret;
>  }
>  
> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo 
> *sn_info,
> -                              const char *name)
> -{
> -    QEMUSnapshotInfo *sn_tab, *sn;
> -    int nb_sns, i, ret;
> -
> -    ret = -ENOENT;
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns < 0)
> -        return ret;
> -    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;
> -        }
> -    }
> -    g_free(sn_tab);
> -    return ret;
> -}
> -
>  /*
>   * Deletes snapshots of a given name in all opened images.
>   */
> @@ -2063,7 +2041,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, name) >= 0)
>          {
>              ret = bdrv_snapshot_delete(bs, name);
>              if (ret < 0) {
> @@ -2123,7 +2101,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);
> +        ret = bdrv_snapshot_find(bs, old_sn, name, name);
>          if (ret >= 0) {
>              pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
>              pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> @@ -2214,7 +2192,7 @@ int load_vmstate(const char *name)
>      }
>  
>      /* Don't even try to load empty VM states */
> -    ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
> +    ret = bdrv_snapshot_find(bs_vm_state, &sn, name, name);
>      if (ret < 0) {
>          return ret;
>      } else if (sn.vm_state_size == 0) {
> @@ -2238,7 +2216,7 @@ int load_vmstate(const char *name)
>              return -ENOTSUP;
>          }
>  
> -        ret = bdrv_snapshot_find(bs, &sn, name);
> +        ret = bdrv_snapshot_find(bs, &sn, name, name);
>          if (ret < 0) {
>              error_report("Device '%s' does not have the requested snapshot 
> '%s'",
>                             bdrv_get_device_name(bs), name);
> @@ -2344,7 +2322,7 @@ 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);
> +                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);

Here we searched for name or ID previously, now we search only for the
ID. We will not miss any snapshot because we only search the snapshot
that we already have and it will always the ID that it has.

We could find duplicates before using the name, so this is in fact a bug
fix and should be a separate patch.

But then, I don't understand the whole logic in do_info_snapshots(). Why
does list all snapshots and then check for each snapshot if it's really
there by indirectly calling bdrv_snapshot_list() again? Doesn't make a
whole lot of sense to me.

Kevin