[Top][All Lists]

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

Re: [RFC] error: auto propagated local_err

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [RFC] error: auto propagated local_err
Date: Fri, 20 Sep 2019 11:46:07 +0000

18.09.2019 16:02, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> Here is a proposal (three of them, actually) of auto propagation for
> local_err, to not call error_propagate on every exit point, when we
> deal with local_err.
> It also may help make Greg's series[1] about error_append_hint smaller.
> See definitions and examples below.
> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> it, the idea will touch same code (and may be more).
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html

OK, seems a kind of consensus is here, and looks like

#define MAKE_ERRP_SAFE() \
g_auto(ErrorPropagationStruct) __auto_errp_prop = {.errp = errp}; \
Error **__local_errp_unused __attribute__ ((unused)) = \
     (errp = (errp == NULL || *errp == error_fatal ? \
              &__auto_errp_prop.local_error : errp))

[I also suggest to call it ERRP_FUNCTION_BEGIN, in case we'll enhance it
in future (for example bring call stack into Error)]

And MAKE_ERRP_SAFE assumed to be used in all errp-functions.

Now, there are still several things to discuss:

1. Some functions want to do error_free(local_err), 
error_report_err(local_err), or
like this, when they decide that error should not be propagated.

What to do with them? I suggest to make corresponding functions

error_free_errp, error_report_errp, warn_report_ferrp, etc, with Error **errp 
which calls corresponding  Error* function and then set pointer to 0. Then our 
cleanup will do nothing, as desired.

2. Some functions tends to error_free(*errp) or error_report_err(*errp). So, 
they don't
use errp to return error, but instead they want to handle errp somehow:
   - is used only in two places to trace errp, so it may be inlined

   - is a wrapper used in more than 80 places, to do error_reportf_err(*errp, 
"Error: ");
(hmm, bad that it doesn't set *errp to ZERO after it)

what do do with it? inline too?

or, maybe rename errp parameter to "Error **filled_errp", to not match our 

(api function error_free_or_abort is with same behavior)

Just bugs:

3. Wow, fit_load_fdt() just error_free(*errp) in wrong way! It must set then 
*errp = NULL,
but it doesn't do it. And qmp_query_cpu_def() is correct example of this thing -
these functions definitely wants error_free_errp function. But 
incorrectly dereference errp (it may be NULL!)

4. A lot of functions in target/s390x/cpu_models.c just dereference errp to 
check error

5. file_ram_alloc has funny patter to check error:
     if (mem_prealloc) {
         os_mem_prealloc(fd, area, memory, ms->smp.cpus, errp);
         if (errp && *errp) {
             qemu_ram_munmap(fd, area, memory);
             return NULL;

Seems nothing interesting more. I used this coccinelle script
identifier fn;

  fn(..., Error **errp)
*    *errp

to search by this commend:

git grep -l 'Error \*\*errp' | xargs grep -l '[^*]\*errp' | xargs spatch 
--sp-file script

Best regards,

reply via email to

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