From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47535) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVHOL-0006JV-EB for qemu-devel@nongnu.org; Thu, 25 Apr 2013 04:19:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVHOF-0006HI-6V for qemu-devel@nongnu.org; Thu, 25 Apr 2013 04:18:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28724) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVHOE-0006H4-Tz for qemu-devel@nongnu.org; Thu, 25 Apr 2013 04:18:51 -0400 Message-ID: <5178E6E5.2070301@redhat.com> Date: Thu, 25 Apr 2013 10:18:45 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <51784E0E.9090607@redhat.com> <5178D13B.9040606@redhat.com> In-Reply-To: <5178D13B.9040606@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, armbru@redhat.com, qemu-devel@nongnu.org, xiawenc@linux.vnet.ibm.com, lcapitulino@redhat.com On 25.4.2013 08:46, Pavel Hrdina wrote: > 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 > Well, this doesn't work too. I'll stick with the bool return value and the bool ret variable. Pavel