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

[Wenchao Xia]

于 2013-6-11 17:25, Stefan Hajnoczi 写道:
> 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
  will rename it.

> >   {
> > -    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 {}.
 OK.

> > -    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.
  shouldn't following line in this patch changed the prototype?

--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -157,7 +157,10 @@ struct BlockDriver {
                                 QEMUSnapshotInfo *sn_info);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                               const char *snapshot_id);
-    int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char 
*snapshot_id);
+    int (*bdrv_snapshot_delete)(BlockDriverState *bs,
+                                const char *snapshot_id,
+                                const char *name,
+                                Error **errp);

  let me retry.

> >   {
> >       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.
  OK, will check it.


--
Best Regards

Wenchao Xia