[Qemu-devel] Error ** parameter conventions (was: [PATCH] qemu-sockets: Fix assertion failure)

[Markus Armbruster]

[Note cc: Luiz]

Kevin Wolf <address@hidden> writes:

> inet_connect_opts() tries all possible addrinfos returned by
> getaddrinfo(). If one fails with an error, the next one is tried. In
> this case, the Error should be discarded because the whole operation is
> successful if another addrinfo from the list succeeds; and if it
> doesn't, setting an already set Error will trigger an assertion failure.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  util/qemu-sockets.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 1350ccc..32e609a 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>      }
>  
>      for (e = res; e != NULL; e = e->ai_next) {
> +
> +        /* Overwriting errors isn't allowed, so clear any error that may have
> +         * occured in the previous iteration */
> +        if (error_is_set(errp)) {
> +            error_free(*errp);
> +            *errp = NULL;
> +        }
> +
>          if (connect_state != NULL) {
>              connect_state->current_addr = e;
>          }

I'm not sure this is the proper solution, but that could be just my
uncertainty on proper Error usage.

The convention as I understand it is that a function stores whatever
error it wants to return through its errp parameter, with
error_set(errp, ...).  error_set() does nothing when passed a null errp.
This lets callers ignore errors without fuss.

I guess we don't want callers to pass a non-null errp with *errp != NULL
ever, but I don't think that's spelled out anywhere.  I suspect code is
generally unprepared for such usage.

Your patch *clears* errors through its errp parameter.  I'm not saying
it's wrong, I just spooks poor ignorant me.

My gut feeling: as soon as we go beyond utterly trivial with errors,
it's best to put them in local variables, and error_propagate() to the
parameter only at the end.

Let's review some usage patterns, in the hope of learning more.

    /*
     * Do something on @arg.
     * On error, set address@hidden if @errp isn't null.
     */
    void do_something(Argument *arg, Error **errp);

    // ignoring errors:
    do_something(arg, NULL);

    // checking and handling errors locally:
    Error *local_err = NULL;
    do_something(arg, &local_err);
    if (local_err) {    // some prefer error_is_set(local_err) here,
                        // but I think that only muddies the waters
        // error handling goes here
        error_free(local_err);
    }

    // propagating to caller via parameter Error *errp:
    Error *local_err = NULL;
    do_something(arg, &local_err);
    if (local_err) {
        // local error handling goes here
        error_propagate(errp, local_err);
    }

    // same, but "simpler" (except it doesn't work):
    do_something(arg, errp);
    if (error_is_set(errp)) {   // BUG: doesn't work when !errp
        // local error handling goes here
    }

    // same, when no local error handling is wanted:
    do_something(arg, errp);

    // but it works only once:
    do_something(arg1, errp);
    do_something(arg2, errp);  // BUG: trips assert() when both fail
    // pity

Let me stress: beware of error_is_set()!  If the argument may or may not
be null, the result is *worthless*.  If it can't be null, you could just
as well test the argument directly.  If it must be null, the result is
always false.

More patterns:
    
    // accumulating errors, first error wins
    Error *err = NULL;
    while (...) {
        Error *local_err = NULL:
        do_something(arg, &local_err);
        if (local_err) {
            // local error handling goes here
            error_propagate(&err, local_err);
        }
    }
    // err contains the first error
    // need to free or propagate it

    // same, but "simpler" (except it doesn't work)
    Error *err = NULL;
    while (...) {
        do_something(arg, &err);// BUG: trips assert() on second error
        if (err) {              // BUG: always true after first error
            // local error handling goes here
        }
    }

    // can omit err when propagating, just replace it by errp:
    // (works because error_propagate() is designed for this)
    while (...) {
        Error *local_err = NULL:
        do_something(arg, &local_err);
        if (local_err) {
            // local error handling goes here
            error_propagate(errp, local_err);
        }
    }
    // errp contains the first error

    // accumulating errors, last error wins
    Error *err = NULL;
    while (...) {
        Error *local_err = NULL:
        do_something(arg, &local_err);
        if (local_err) {
            // local error handling goes here
            // if we do the following often, we should perhaps provide a
            // function for it, just like error_propagate(), only "last
            // one wins" instead of "first one wins"
            if (err) {
                error_free(err);
            }
            err = local_err;
        }
    }
    // err contains the last error
    // need to free or propagate it

    // can omit err when propagating, but need to be careful, because
    // errp may be null:
    while (...) {
        Error *local_err = NULL:
        do_something(arg, &local_err);
        if (local_err) {
            // local error handling goes here
            if (error_is_set(errp)) {
                // exploits that error_is_set(errp) implies errp != NULL
                error_free(*errp);
                *errp = NULL;
            }
            error_propagate(errp, local_err);
        }
    }

Just found a tolerable use of error_is_set().  I'd prefer a function
error_clear(errp) to clear through a possibly null errp.

Still more patterns:

    // try a number of args, until one works
    Error *local_err = NULL;
    for (arg = first_one(); arg; arg = next_one()) {
        if (local_err) {
            error_free(local_err);
            local_err = NULL;
        }
        do_something(arg, &local_err);
        if (!local_err) {
            break;              // arg worked
        }
        // local error handling goes here
    }
    if (local_err) {
        // local error handling goes here
        // need to free or propagate local_err
    } else {
        // arg worked
    }

We can't omit local_err when propagating, because with just errp, we
can't test whether do_something() succeeded.

With a function that return a value that can be tested:

    /*
     * Get @arg's foo.
     * On success, return foo.
     * On failure, return null, and set address@hidden if @errp isn't null.
     */
    Foo *get_foo(Argument *arg, Error *errp)

we can omit local_err:

    // try a number of args, until one works
    for (arg = first_one(); arg; arg = next_one()) {
        if (error_is_set(errp)) {
            // exploits that error_is_set(errp) implies errp != NULL
            error_free(*errp);
            *errp = NULL;
        }
        foo = get_foo(arg, errp);
        if (foo) {
            break;              // arg worked
        }
        // local error handling goes here
    }
    if (foo) {
        // arg worked
    } else {
        // local error handling goes here
        // need to free or propagate local_err
    }

This is roughly how your patch works.

Functions like get_foo() spook me, because when I do

    local_err = NULL;
    foo = get_foo(arg, &local_err);
    if (!foo) {
        // fail
        return;
    }
    // success
    // use foo (surely not null)

I worry about local_err being null at // fail, or non-null (and leaked)
at // success.  When I do

    local_err = NULL;
    foo = get_foo(arg, &local_err);
    if (local_err) {
        // fail
        return;
    }
    // success
    // use foo

I worry about foo being null at // success, or non-null (and leaked) at
// fail.

Perhaps I'm too easily worried.