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.