From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:36567)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1Zw5Ae-0002gP-R4
	for qemu-devel@nongnu.org; Tue, 10 Nov 2015 04:25:00 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1Zw5Aa-0005ND-UQ
	for qemu-devel@nongnu.org; Tue, 10 Nov 2015 04:24:56 -0500
From: Markus Armbruster <armbru@redhat.com>
References: <1444985866-12969-1-git-send-email-wency@cn.fujitsu.com>
	<1444985866-12969-4-git-send-email-wency@cn.fujitsu.com>
	<w511tbzxmh8.fsf@maestria.local.igalia.com>
	<56419B8C.5000204@cn.fujitsu.com>
Date: Tue, 10 Nov 2015 10:24:44 +0100
In-Reply-To: <56419B8C.5000204@cn.fujitsu.com> (Wen Congyang's message of
	"Tue, 10 Nov 2015 15:23:56 +0800")
Message-ID: <877flq1a1v.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH v6 3/4] qmp: add monitor command to
	add/remove a child
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Wen Congyang <wency@cn.fujitsu.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Alberto Garcia <berto@igalia.com>, zhanghailiang <zhang.zhanghailiang@huawei.com>, qemu block <qemu-block@nongnu.org>, Jiang Yunhong <yunhong.jiang@intel.com>, Dong Eddie <eddie.dong@intel.com>, qemu devel <qemu-devel@nongnu.org>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Gonglei <arei.gonglei@huawei.com>, Stefan Hajnoczi <stefanha@redhat.com>, Yang Hongyang <yanghy@cn.fujitsu.com>

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 11/09/2015 10:42 PM, Alberto Garcia wrote:
>> Sorry again for the late review, here are my comments:
>> 
>> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
>>> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
>>> +                           bool has_child, const char *child,
>>> +                           bool has_new_node, const char *new_node,
>>> +                           Error **errp)
>> 
>> You are using different names for the parameters here: 'op', 'parent',
>> 'child', 'new_node'; in the JSON file the first and last one are named
>> 'operation' and 'node'.
>
> OK, I will fix it in the next version
>
>> 
>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>> +    if (!parent_bs) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>> 
>> You don't need to change it if you don't want but you can use errp
>> directly here and spare the error_propagate() call.
>
> Too many codes in qemu use local_err and error_propagate(). I think
> errp can be NOT NULL here(in which case?).

It's usually advisable not to rely on "all callers pass non-null value
to parameter errp" arguments, because they're non-local and tend to be
brittle.

error.h attempts to provide guidance:

 * Receive an error and pass it on to the caller:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *         error_propagate(errp, err);
 *     }
 * where Error **errp is a parameter, by convention the last one.
 *
 * Do *not* "optimize" this to
 *     foo(arg, errp);
 *     if (*errp) { // WRONG!
 *         handle the error...
 *     }
 * because errp may be NULL!
 *
 * But when all you do with the error is pass it on, please use
 *     foo(arg, errp);
 * for readability.

Since all you do with local_err in the quoted code snippet is pass it
on, the last paragraph applies, and you can simplify to:

    parent_bs = bdrv_lookup_bs(parent, parent, errp);
    if (!parent_bs) {
        return;
    }

Whether errp can be null doesn't matter.

[...]