From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54586) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVFwn-0003AY-CE for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:46:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVFwl-0001Cq-Tb for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:46:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44796) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVFwl-0001Cj-MT for qemu-devel@nongnu.org; Thu, 25 Apr 2013 02:46:23 -0400 Message-ID: <5178D13B.9040606@redhat.com> Date: Thu, 25 Apr 2013 08:46:19 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <51784E0E.9090607@redhat.com> In-Reply-To: <51784E0E.9090607@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed 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: Eric Blake Cc: kwolf@redhat.com, xiawenc@linux.vnet.ibm.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On 24.4.2013 23:26, Eric Blake wrote: > On 04/24/2013 09:32 AM, Pavel Hrdina 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 > > double-typo and grammar: > s/also/also a/ > s/containt error messeage/contain the error message/ > >> 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) > > I'd add a FIXME comment here documenting that you intend to remove the > old_match parameter after all callers have been updated to the new > semantics. > >> { >> QEMUSnapshotInfo *sn_tab, *sn; >> - int nb_sns, i, ret; >> + int nb_sns, i; >> + bool found = false; > > Bikeshedding: I don't think you need this variable, if you would instead > do... > >> + >> + 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; > > return false; > >> + } >> + >> + if (nb_sns == 0) { >> + error_setg(errp, "Device has no snapshots"); >> + return found; > > return false; > >> + } > > *sn_info = NULL; You cannot do that. It should be sn_info = NULL, but if you look at the usage of the bdrv_snapshot_find you will see that you cannot do that too. And the same applies ... > >> + >> + 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; > > Drop this assignment and others like it... > >> + 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; >> + } >> } >> } >> + >> + if (!found) { > > use 'if (*sn_info)' to this usage and ... > >> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); >> + } >> + >> g_free(sn_tab); >> - return ret; >> + return found; > > return *sn_info != NULL; this one. > >> } > > If you _do_ decide to keep the boolean variable instead of hard-coding a > false return and avoiding redundancy by using other variables to > determine the result, then at least s/found/ret/, because I find 'return > found' as a way to intentionally fail rather odd-looking. > > At any rate, I can live with this logic, and all the conversions of > existing call sites properly passed the given name, NULL id, and true > for old_match semantics; along with optional deciding whether to pass > NULL or a local error based on whether it would ignore lookup failure or > propagate it as a failure of the higher-level operation that needed a > lookup. > You are right that avoiding redundancy is better and I just think up a new solution. static QEMUSnapshotInfo *bdrv_snapshot_find(BlockDriverState *bs, const char *name, const char *id, Error **errp, bool old_match /*FIXME*/) And the bdrv_snapshot_find will return QEMUSnapshotInfo* on success and on error it will set the error message and also return NULL. Pavel