Re: [Qemu-devel] [PATCH 06/11] snapshot: distinguish id and name in snapshot delete

[Stefan Hajnoczi <stefanha@gmail.com>]

On Sat, Jun 08, 2013 at 02:58:02PM +0800, Wenchao Xia wrote:
>  static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)

I suggest renaming the argument to make it less confusing: const char *name_or_id

>  {
> -    BDRVQcowState *s = bs->opaque;
> -    int i, ret;
> +    int ret;
>  
> -    ret = find_snapshot_by_id(bs, name);
> +    ret = find_snapshot_by_id_and_name(bs, name, NULL);
>      if (ret >= 0)
>          return ret;

Since you're touching the other lines in this function you could add {}.

> -    for(i = 0; i < s->nb_snapshots; i++) {
> -        if (!strcmp(s->snapshots[i].name, name))
> -            return i;
> -    }
> -    return -1;
> +    return find_snapshot_by_id_and_name(bs, NULL, name);
>  }
>  
>  /* if no id is provided, a new one is constructed */
> @@ -333,7 +347,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>      }
>  
>      /* Check that the ID is unique */
> -    if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
> +    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
>          return -EEXIST;
>      }
>  
> @@ -530,15 +544,21 @@ fail:
>      return ret;
>  }
>  
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +int qcow2_snapshot_delete(BlockDriverState *bs,
> +                          const char *snapshot_id,
> +                          const char *name,
> +                          Error **errp)

This patch will fail to compile.  You haven't changed the
.bdrv_snapshot_delete() prototype.

Please make sure every patch compiles.

>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot sn;
>      int snapshot_index, ret;
>  
>      /* Search the snapshot */
> -    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> +    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
>      if (snapshot_index < 0) {
> +        error_setg(errp,
> +                   "Can't find a snapshot with ID %s and name %s",
> +                   snapshot_id, name);

Are both snapshot_id and name guaranteed to be non-NULL here?  It is
dangerous to pass NULL strings to sprintf() functions.  IIRC some libc
implementations will crash, others will print "(null)" like Linux.