From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59577) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VijM7-0000aH-K9 for qemu-devel@nongnu.org; Tue, 19 Nov 2013 06:20:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VijM1-0004F2-BG for qemu-devel@nongnu.org; Tue, 19 Nov 2013 06:20:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:63850) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VijM1-0004Ex-30 for qemu-devel@nongnu.org; Tue, 19 Nov 2013 06:20:25 -0500 Date: Tue, 19 Nov 2013 12:20:20 +0100 From: Kevin Wolf Message-ID: <20131119112020.GC4040@dhcp-200-207.str.redhat.com> References: <1384121021-24815-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1384121021-24815-2-git-send-email-xiawenc@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1384121021-24815-2-git-send-email-xiawenc@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH V5 1/5] snapshot: distinguish id and name in load_tmp List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: pbonzini@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Am 10.11.2013 um 23:03 hat Wenchao Xia geschrieben: > Since later this function will be used so improve it. The only caller of it > now is qemu-img, and it is not impacted by introduce function > bdrv_snapshot_load_tmp_by_id_or_name() that call bdrv_snapshot_load_tmp() > twice to keep old search logic. bdrv_snapshot_load_tmp_by_id_or_name() return > int to let caller know the errno, and errno will be used later. > Also fix a typo in comments of bdrv_snapshot_delete(). > > Signed-off-by: Wenchao Xia > --- > block/qcow2-snapshot.c | 16 ++++++++++- > block/qcow2.h | 5 +++- > block/snapshot.c | 60 ++++++++++++++++++++++++++++++++++++++++++-- > include/block/block_int.h | 4 ++- > include/block/snapshot.h | 7 ++++- > qemu-img.c | 8 ++++- > 6 files changed, 90 insertions(+), 10 deletions(-) > > diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c > index 3529c68..aeb5efd 100644 > --- a/block/qcow2-snapshot.c > +++ b/block/qcow2-snapshot.c > @@ -675,7 +675,10 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) > return s->nb_snapshots; > } > > -int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name) > +int qcow2_snapshot_load_tmp(BlockDriverState *bs, > + const char *snapshot_id, > + const char *name, > + Error **errp) > { > int i, snapshot_index; > BDRVQcowState *s = bs->opaque; > @@ -683,12 +686,17 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name) > uint64_t *new_l1_table; > int new_l1_bytes; > int ret; > + const char *device = bdrv_get_device_name(bs); This is wrong, low-level functions shouldn't need the device name for anything. > assert(bs->read_only); > > /* Search the snapshot */ > - snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_name); > + 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' " > + "on device '%s'", > + STR_OR_NULL(snapshot_id), STR_OR_NULL(name), device); > return -ENOENT; > } > sn = &s->snapshots[snapshot_index]; I think we already discussed the same thing in the context of a different series: The caller knows which device and which snapshot name or ID he used. The only information he really needs is: error_setg(errp, "Can't find snapshot"); If in the context of the caller's caller this isn't enough, the caller can add this information. I doubt that it's even necessary in this case. > @@ -699,6 +707,10 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name) > > ret = bdrv_pread(bs->file, sn->l1_table_offset, new_l1_table, new_l1_bytes); > if (ret < 0) { > + error_setg(errp, > + "Failed to read l1 table for snapshot with ID '%s' and name " > + "'%s' on device '%s'", > + sn->id_str, sn->name, device); > g_free(new_l1_table); > return ret; > } > diff --git a/block/qcow2.h b/block/qcow2.h > index 922e190..303eb26 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -488,7 +488,10 @@ int qcow2_snapshot_delete(BlockDriverState *bs, > const char *name, > Error **errp); > int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab); > -int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name); > +int qcow2_snapshot_load_tmp(BlockDriverState *bs, > + const char *snapshot_id, > + const char *name, > + Error **errp); > > void qcow2_free_snapshots(BlockDriverState *bs); > int qcow2_read_snapshots(BlockDriverState *bs); > diff --git a/block/snapshot.c b/block/snapshot.c > index a05c0c0..e51a7db 100644 > --- a/block/snapshot.c > +++ b/block/snapshot.c > @@ -194,7 +194,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs, > * If only @snapshot_id is specified, delete the first one with id > * @snapshot_id. > * If only @name is specified, delete the first one with name @name. > - * if none is specified, return -ENINVAL. > + * if none is specified, return -EINVAL. > * > * Returns: 0 on success, -errno on failure. If @bs is not inserted, return > * -ENOMEDIUM. If @snapshot_id and @name are both NULL, return -EINVAL. If @bs > @@ -265,18 +265,72 @@ int bdrv_snapshot_list(BlockDriverState *bs, > return -ENOTSUP; > } > > +/** > + * Temporarily load an internal snapshot by @snapshot_id and @name. > + * @bs: block device used in the operation > + * @snapshot_id: unique snapshot ID, or NULL > + * @name: snapshot name, or NULL > + * @errp: location to store error > + * > + * If both @snapshot_id and @name are specified, load the first one with > + * id @snapshot_id and name @name. > + * If only @snapshot_id is specified, load the first one with id > + * @snapshot_id. > + * If only @name is specified, load the first one with name @name. > + * if none is specified, return -EINVAL. > + * > + * Returns: 0 on success, -errno on fail. If @bs is not inserted, return > + * -ENOMEDIUM. If @bs is not readonly, return -EINVAL. If @bs did not support > + * internal snapshot, return -ENOTSUP. If qemu can't find one matching @id and s/one/a/ > + * @name, return -ENOENT. If @bs does not support parameter @snapshot_id or > + * @name, return -EINVAL. What do you mean by "bs does not support parameter"? Is this when you specify a name, but the block driver doesn't use names, but only IDs? > If @errp != NULL, it will always be filled on > + * failure. > + */ The rest looks good. Kevin