Re: [RFC] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

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 
parameter,
which calls corresponding  Error* function and then set pointer to 0. Then our 
propagation
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:
vnc_client_io_error
   - is used only in two places to trace errp, so it may be inlined

hmp_handle_error
   - 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 
criteria?

(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 
qmp_query_cpu_def()
incorrectly dereference errp (it may be NULL!)

4. A lot of functions in target/s390x/cpu_models.c just dereference errp to 
check error
also:
build_guest_fsinfo_for_virtual_device
legacy_acpi_cpu_plug_cb
sclp_events_bus_realize
memory_device_get_free_addr
ipmi_isa_realize
isa_ipmi_bt_realize
legacy_acpi_cpu_plug_cb

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,
Vladimir