[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v6 3/4] qmp: add monitor command to

From: Markus Armbruster
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
Date: Tue, 10 Nov 2015 10:24:44 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Wen Congyang <address@hidden> 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

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) {

Whether errp can be null doesn't matter.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]