Wen Congyang <we...@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. [...]