From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48239) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vjfv4-00045e-Gb for qemu-devel@nongnu.org; Thu, 21 Nov 2013 20:52:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vjfuv-0005F6-GO for qemu-devel@nongnu.org; Thu, 21 Nov 2013 20:52:30 -0500 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:44953) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vjfuu-0005Eh-N9 for qemu-devel@nongnu.org; Thu, 21 Nov 2013 20:52:21 -0500 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 22 Nov 2013 11:52:17 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 925C62BB0058 for ; Fri, 22 Nov 2013 12:52:14 +1100 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id rAM1YIwh6095308 for ; Fri, 22 Nov 2013 12:34:25 +1100 Received: from d23av04.au.ibm.com (localhost [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id rAM1q7OK024339 for ; Fri, 22 Nov 2013 12:52:07 +1100 Message-ID: <528EB8C6.2030402@linux.vnet.ibm.com> Date: Fri, 22 Nov 2013 09:52:06 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1384121021-24815-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1384121021-24815-2-git-send-email-xiawenc@linux.vnet.ibm.com> <20131119112020.GC4040@dhcp-200-207.str.redhat.com> In-Reply-To: <20131119112020.GC4040@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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: Kevin Wolf Cc: pbonzini@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com 于 2013/11/19 19:20, Kevin Wolf 写道: > 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. > I remember that, will follow this rule. >> @@ -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/ > OK. >> + * @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? > likely, I mean rbd doesn't support ID. But rbd and snapshot doesn't implement load_tmp, so will remove this comments. >> If @errp != NULL, it will always be filled on >> + * failure. >> + */ > > The rest looks good. > > Kevin >