[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
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.
[...]