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