Re: [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot()

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

>   To make it clear about id and name in searching, the API is changed
> a bit to distinguish them, and caller can choose to search by id or name.
> Searching will be done with higher priority of id. This function also
> returns negative value from bdrv_snapshot_list() instead of -ENOENT on
> error now.
>
> Signed-off-by: Wenchao Xia <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> Reviewed-by: Kevin Wolf <address@hidden>
> ---
>  block/snapshot.c         |   46 
> ++++++++++++++++++++++++++++++++++++++--------
>  include/block/snapshot.h |    2 +-
>  savevm.c                 |   10 +++++-----
>  3 files changed, 44 insertions(+), 14 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index c47a899..7b2026c 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -24,8 +24,20 @@
>  
>  #include "block/snapshot.h"
>  
> +/*
> + * 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.
> + */

Let me try to improve:

/**
 * Look up an internal snapshot by @id, or else by @name.
 * @bs: block device to search
 * @sn_info: location to store information on the snapshot found
 * @id: unique snapshot ID, or NULL
 * @name: snapshot name, or NULL
 *
 * If the snapshot with unique ID @id exists, find it.
 * Else, if snapshots with name @name exists, find one of them.
 *
 * Returns: 0 when a snapshot is found, else -errno.
 */

I'd do two separate functions, one to loop up a snapshot by ID, and one
to look it up by name, because I find them simpler.  Matter of taste.

>  int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> -                       const char *name)
> +                       const char *id, const char *name)
>  {
>      QEMUSnapshotInfo *sn_tab, *sn;
>      int nb_sns, i, ret;
> @@ -33,16 +45,34 @@ int bdrv_snapshot_find(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info,
>      ret = -ENOENT;
>      nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>      if (nb_sns < 0) {
> -        return ret;
> +        return nb_sns;
>      }
> -    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;
> +
> +    /* 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;
> +            }
> +        }
> +    }
> +
> + out:
>      g_free(sn_tab);
>      return ret;
>  }
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index 4ad070c..a047a8e 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -33,5 +33,5 @@
>  #include "block.h"
>  
>  int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> -                       const char *name);
> +                       const char *id, const char *name);
>  #endif
> diff --git a/savevm.c b/savevm.c
> index 88acc38..1d2da99 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2212,7 +2212,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) {

Before your patch: deletes first snapshot sn whose sn->id_str or
sn->name match name.

After your patch: deletes first snapshot sn whose sn->id_str matches
name, or else first snapshot whose sn->name matches name.

Running example: say we have the following two snapshots with rather
unwisely chosen names:

    id_str      name
    1           2
    2           1

Which one will del_existing_snapshots(mon, "2") delete?

Before your patch: first one.

After: second one.

See below for impact.

> @@ -2272,7 +2272,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);

Similar change.

This function is the only user of del_existing_snapshots().

Same example.

Before your patch: "savevm 2" overwrites the first snapshot.

After: it overwrites the second snapshot.

Thus, your patch changes  del_existing_snapshots() 

I'm fine with the change, but I want it noted *prominently* in the
commit message that it can change savevm's interpretation of ambiguous
name arguments.

> @@ -2363,7 +2363,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) {
> @@ -2387,7 +2387,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);

Same example.

Before your patch: "loadvm 2" loads the first snapshot.

After: it loads the second snapshot.

Document in commit message, just like the savevm change.

> @@ -2493,7 +2493,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);
>                  if (ret < 0) {
>                      available = 0;
>                      break;

Different from the other calls: name argument is NULL rather than same
as id argument.  Let's see what difference it makes.

The loop iterates over all snapshots on the "primary" snapshot device
(the one that gets the VM state on savevm).  For each one, it checks
whether all other snapshot devices have that snapshot.

Before your patch: a device has the snapshot when it has one whose
sn->id_str or sn->name match the primary snapshot's id_str.  Nuts :)

After your patch: a device has the snapshot when it has one whose
sn->id_str matches the primary snapshot's id_str.

I'm fine with the change, but it needs to be documented in the commit
message.