From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55930) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVG2t-0006SI-Kz for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:52:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVG2s-0003Ka-HG for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:52:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16410) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVG2s-0003KU-8B for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:52:42 -0400 Message-ID: <5178D2B4.80609@redhat.com> Date: Thu, 25 Apr 2013 08:52:36 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <5178CDB7.8080706@linux.vnet.ibm.com> In-Reply-To: <5178CDB7.8080706@linux.vnet.ibm.com> Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On 25.4.2013 08:31, Wenchao Xia wrote: >> Finding snapshot by a name which could also be an id isn't best way >> how to do it. There will be rewrite of savevm, loadvm and delvm to >> improve the behavior of these commands. The savevm and loadvm will >> have their own patch series. >> >> Now bdrv_snapshot_find takes more parameters. The name parameter will >> be matched only against the name of the snapshot and the same applies >> to id parameter. >> >> There is one exception. If you set the last parameter, the name parameter >> will be matched against the name or the id of a snapshot. This exception >> is only for backward compatibility for other commands and it will be >> dropped after all commands will be rewritten. >> >> We only need to know if that snapshot exists or not. We don't care >> about any error message. If snapshot exists it returns TRUE otherwise >> it returns FALSE. >> >> There is also new Error parameter which will containt error messeage if >> something goes wrong. >> >> Signed-off-by: Pavel Hrdina >> --- >> savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------ >> 1 file changed, 67 insertions(+), 26 deletions(-) >> >> diff --git a/savevm.c b/savevm.c >> index ba97c41..1622c55 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -2262,26 +2262,66 @@ out: >> return ret; >> } >> >> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> - const char *name) >> +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> + const char *name, const char *id, Error **errp, >> + bool old_match) > suggest directly drop old_match parameter here, or squash patch 12 > into this one, mark the change in commit message, then the logic will > be clearer. > Personally hope to place parameter *id before *name. That make sense, I'll change it. > >> { >> QEMUSnapshotInfo *sn_tab, *sn; >> - int nb_sns, i, ret; >> + int nb_sns, i; >> + bool found = false; >> + >> + assert(name || id); >> >> - ret = -ENOENT; >> nb_sns = bdrv_snapshot_list(bs, &sn_tab); >> - if (nb_sns < 0) >> - return ret; >> - for(i = 0; i < nb_sns; i++) { >> + if (nb_sns < 0) { >> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list"); >> + return found; >> + } >> + >> + if (nb_sns == 0) { >> + error_setg(errp, "Device has no snapshots"); >> + return found; >> + } > suggest not set errp here, which can be used to tip exception, but > having not found one is a normal case. You don't have to use the error message. It is set as the error message but actually is more like an announcement. > >> + >> + for (i = 0; i < nb_sns; i++) { >> sn = &sn_tab[i]; >> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { >> - *sn_info = *sn; >> - ret = 0; >> - break; >> + if (name && id) { >> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + found = true; >> + break; >> + } >> + } else if (name) { >> + /* for compatibility for old bdrv_snapshot_find call >> + * will be removed */ >> + if (old_match) { >> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + found = true; >> + break; >> + } >> + } else { >> + if (!strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + found = true; >> + break; >> + } >> + } >> + } else if (id) { >> + if (!strcmp(sn->id_str, id)) { >> + *sn_info = *sn; >> + found = true; >> + break; >> + } >> } >> } > suggest change the sequence: > > if (name && id) { > for () { > } > } else if (name){ > for () { > } > } else if (id) { > for () { > } > } > slightly faster, and make logic more clear. Thanks for suggest, I'll change the logic. > >> + >> + if (!found) { >> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); > suggest not to set error, since it is a normal case. The same comment as for "Device has no snapshots". Pavel