Quoting Markus Armbruster (2014-04-28 15:27:49) > Using error_is_set(ERRP) to find out whether a function failed is > either wrong, fragile, or unnecessarily opaque. It's wrong when ERRP > may be null, because errors go undetected when it is. It's fragile > when proving ERRP non-null involves a non-local argument. Else, it's > unnecessarily opaque (see commit 84d18f0). > > The error_is_set(errp) in do_qmp_dispatch() is merely fragile, because > the caller never passes a null errp argument. > > Make the code more robust and more obviously correct: receive the > error in a local variable, then propagate it through the parameter. > > Signed-off-by: Markus Armbruster <[email protected]> > Reviewed-by: Eric Blake <[email protected]>
Reviewed-by: Michael Roth <[email protected]> > --- > qapi/qmp-dispatch.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c > index 187af56..168b083 100644 > --- a/qapi/qmp-dispatch.c > +++ b/qapi/qmp-dispatch.c > @@ -62,6 +62,7 @@ static QDict *qmp_dispatch_check_obj(const QObject > *request, Error **errp) > > static QObject *do_qmp_dispatch(QObject *request, Error **errp) > { > + Error *local_err = NULL; > const char *command; > QDict *args, *dict; > QmpCommand *cmd; > @@ -93,13 +94,13 @@ static QObject *do_qmp_dispatch(QObject *request, Error > **errp) > > switch (cmd->type) { > case QCT_NORMAL: > - cmd->fn(args, &ret, errp); > - if (!error_is_set(errp)) { > - if (cmd->options & QCO_NO_SUCCESS_RESP) { > - g_assert(!ret); > - } else if (!ret) { > - ret = QOBJECT(qdict_new()); > - } > + cmd->fn(args, &ret, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + } else if (cmd->options & QCO_NO_SUCCESS_RESP) { > + g_assert(!ret); > + } else if (!ret) { > + ret = QOBJECT(qdict_new()); > } > break; > } > -- > 1.8.1.4
