From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55300) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPrft-0000rI-Lc for qemu-devel@nongnu.org; Tue, 18 Mar 2014 06:55:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPrfm-0006yE-N6 for qemu-devel@nongnu.org; Tue, 18 Mar 2014 06:55:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24971) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPrfm-0006jE-GC for qemu-devel@nongnu.org; Tue, 18 Mar 2014 06:55:06 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2IAt01w000375 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 18 Mar 2014 06:55:00 -0400 Date: Tue, 18 Mar 2014 11:54:59 +0100 From: Kevin Wolf Message-ID: <20140318105459.GK4607@noname.str.redhat.com> References: <1395133219-8009-1-git-send-email-kwolf@redhat.com> <87txavrf7e.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87txavrf7e.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH] block: Remove -errno return value from bdrv_assign_node_name List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, stefanha@redhat.com Am 18.03.2014 um 11:09 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > It takes an errp argument. That's enough for error handling. > > > > Signed-off-by: Kevin Wolf > > --- > > block.c | 21 ++++++++++----------- > > 1 file changed, 10 insertions(+), 11 deletions(-) > > > > diff --git a/block.c b/block.c > > index acb70fd..b4f3f77 100644 > > --- a/block.c > > +++ b/block.c > > @@ -783,18 +783,18 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags) > > return open_flags; > > } > > > > -static int bdrv_assign_node_name(BlockDriverState *bs, > > - const char *node_name, > > - Error **errp) > > +static void bdrv_assign_node_name(BlockDriverState *bs, > > + const char *node_name, > > + Error **errp) > > { > > if (!node_name) { > > - return 0; > > + return; > > } > > > > /* empty string node name is invalid */ > > if (node_name[0] == '\0') { > > error_setg(errp, "Empty node name"); > > - return -EINVAL; > > + return; > > } > > > > /* takes care of avoiding namespaces collisions */ > > @@ -807,14 +807,12 @@ static int bdrv_assign_node_name(BlockDriverState *bs, > > /* takes care of avoiding duplicates node names */ > > if (bdrv_find_node(node_name)) { > > error_setg(errp, "Duplicate node name"); > > - return -EINVAL; > > + return; > > } > > > > /* copy node name into the bs and insert it into the graph list */ > > pstrcpy(bs->node_name, sizeof(bs->node_name), node_name); > > QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list); > > - > > - return 0; > > } > > > > /* > > @@ -849,9 +847,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, > > trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name); > > > > node_name = qdict_get_try_str(options, "node-name"); > > - ret = bdrv_assign_node_name(bs, node_name, errp); > > - if (ret < 0) { > > - return ret; > > + bdrv_assign_node_name(bs, node_name, &local_err); > > + if (error_is_set(&local_err)) { > > + error_propagate(errp, local_err); > > + return -EINVAL; > > } > > qdict_del(options, "node-name"); > > Please use 'if (local_err)' instead of 'if (error_is_set(&local_err))'. > See commit 84d18f0. I can do that; this patch predates your change and I just dug it out from an old branch. Note that there are still many places that use error_is_set() where it's not necessary, including documentation that should show how to do things right, e.g. docs/writing-qmp-commands.txt. (By the way, would the actual problem with Coverity in commit 84d18f0 not be fixed more easily and for all instances by making error_is_set() a static inline function in the header?) What was the proper use case for error_is_set() again? Or can we get rid of it? As long as it's there, you'll keep getting new offenders. Kevin