Re: [PATCH v4 00/31] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

08.10.2019 12:39, Greg Kurz wrote:
> On Tue, 8 Oct 2019 08:41:08 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 08.10.2019 10:30, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> Hi all!
>>>>
>>>> Here is a proposal of auto propagation for local_err, to not call
>>>> error_propagate on every exit point, when we deal with local_err.
>>>>
>>>> There are also two issues with errp:
>>>>
>>>> 1. error_fatal & error_append_hint/error_prepend: user can't see this
>>>> additional info, because exit() happens in error_setg earlier than info
>>>> is added. [Reported by Greg Kurz]
>>>
>>> How is this series related to Greg's "[PATCH 00/17] Fix usage of
>>> error_append_hint()"?  Do we need both?
>>
>> These series is a substitution for Greg's. Still, there are problems with
>> automation, which Greg pointed in 21/31, and I don't know what to do next.
>>
>> May be, just continue to review patches and fix them by hand. May be try to
>> improve automation...
>>
> 
> The feeling I have after working on my series is that the lines that deal
> with errors are mixed up with the functional code in a variety of ways.
> That makes it very difficult if not impossible to come with code patterns
> suitable for a 100% automated solution IMHO.
> 
> My questioning is more around the semantics of error_fatal actually. What
> does passing &error_fatal gives us over passing &local_err and calling
> error_report_err()+exit(), apart from breaking error_append_hint() and
> error_prepend() ?

As I understand, the only benefit is one line instead of four:

func(..., &error_fatal);

instead of

func(..., &local_err);
if (local_err) {
     exit(1);
}

But, keeping in mind all difficulties about these series... We can consider
conversion error_fatal -> local_err too. It seems simple to do with a coccinelle
script, I can send another automatic series to look at it.


Hmm, some ideas around this:

1. We can generate _fatal versions of functions by python script (we'll call it from Makefile, we have a lot of generated code anyway).

and convert
func(..., &local_err); to

func_fatal(...);

2. Use macro like

#define FATAL(func, ...) do { Error *__fatal_err = NULL; func(__VA_ARGS__ __VA_OPT(,), &__fatal_err); if (__fatal_err) { error_report(__fatal_err); exit(1); } } while (0)

and convert
func(..., &local_err); to

FATAL(func, ...);

> 
>>>
>>>> 2. error_abort & error_propagate: when we wrap
>>>> error_abort by local_err+error_propagate, resulting coredump will
>>>> refer to error_propagate and not to the place where error happened.
>>>> (the macro itself don't fix the issue, but it allows to [3.] drop all
>>>> local_err+error_propagate pattern, which will definitely fix the issue)
>>>> [Reported by Kevin Wolf]
>>>>
>>>> Still, applying new macro to all errp-functions is a huge task, which is
>>>> impossible to solve in one series.
>>>>
>>>> So, here is a minimum: solve only [1.], by adding new macro to all
>>>> errp-functions which wants to call error_append_hint.
>>>>
>>>> v4;
>>>> 02: - check errp to be not NULL
>>>>       - drop Eric's r-b
>>>> 03: add Eric's r-b
>>>> 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin]
>>>>       - improve comment and commit msg, mention
>>>>         error_prepend
>>>> 05: - handle error_prepend too
>>>>       - use new macro name
>>>>       - drop empty line at the end
>>>>
>>>> commit message for auto-generated commits updated,
>>>> commits regenerated.
>>>>
>>>> I'll use cc-cmd to cc appropriate recipients per patch, still
>>>> cover-letter together with 04-06 patches should be interesting for
>>>> all:
>>> [...]
>>>
>>> Big series; let me guess its structure.
>>>
>>>> Vladimir Sementsov-Ogievskiy (31):
>>>>     errp: rename errp to errp_in where it is IN-argument
>>>>     hw/core/loader-fit: fix freeing errp in fit_load_fdt
>>>>     net/net: fix local variable shadowing in net_client_init
>>>
>>> Preparations.
>>>
>>>>     error: auto propagated local_err
>>>
>>> The new idea.
>>>
>>>>     scripts: add script to fix error_append_hint/error_prepend usage
>>>>     python: add commit-per-subsystem.py
>>>
>>> Scripts to help you apply it.
>>>
>>>>     s390: Fix error_append_hint/error_prepend usage
>>>>     ARM TCG CPUs: Fix error_append_hint/error_prepend usage
>>>>     PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage
>>>>     arm: Fix error_append_hint/error_prepend usage
>>>>     SmartFusion2: Fix error_append_hint/error_prepend usage
>>>>     ASPEED BMCs: Fix error_append_hint/error_prepend usage
>>>>     Boston: Fix error_append_hint/error_prepend usage
>>>>     PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage
>>>>     PCI: Fix error_append_hint/error_prepend usage
>>>>     SCSI: Fix error_append_hint/error_prepend usage
>>>>     USB: Fix error_append_hint/error_prepend usage
>>>>     VFIO: Fix error_append_hint/error_prepend usage
>>>>     vhost: Fix error_append_hint/error_prepend usage
>>>>     virtio: Fix error_append_hint/error_prepend usage
>>>>     virtio-9p: Fix error_append_hint/error_prepend usage
>>>>     XIVE: Fix error_append_hint/error_prepend usage
>>>>     block: Fix error_append_hint/error_prepend usage
>>>>     chardev: Fix error_append_hint/error_prepend usage
>>>>     cmdline: Fix error_append_hint/error_prepend usage
>>>>     QOM: Fix error_append_hint/error_prepend usage
>>>>     Migration: Fix error_append_hint/error_prepend usage
>>>>     Sockets: Fix error_append_hint/error_prepend usage
>>>>     nbd: Fix error_append_hint/error_prepend usage
>>>>     PVRDMA: Fix error_append_hint/error_prepend usage
>>>>     ivshmem: Fix error_append_hint/error_prepend usage
>>>
>>> Applying it.
>>>
>>> Correct?
>>>
>>
>> Yes
>>
>>
> 


-- 
Best regards,
Vladimir