[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/11] snapshot: distinguish id and name in snap
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 06/11] snapshot: distinguish id and name in snapshot delete |
Date: |
Tue, 11 Jun 2013 11:25:28 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
[Qemu-devel] [PATCH 06/11] snapshot: distinguish id and name in snapshot delete, Wenchao Xia, 2013/06/08
[Qemu-devel] [PATCH 07/11] qmp: add internal snapshot support in qmp_transaction, Wenchao Xia, 2013/06/08
[Qemu-devel] [PATCH 08/11] qmp: add interface blockdev-snapshot-internal-sync, Wenchao Xia, 2013/06/08
[Qemu-devel] [PATCH 09/11] qmp: add interface blockdev-snapshot-delete-internal-sync, Wenchao Xia, 2013/06/08
[Qemu-devel] [PATCH 10/11] hmp: add interface hmp_snapshot_internal_blkdev, Wenchao Xia, 2013/06/08