Re: [RFC v2 0/9] error: auto propagated local_err

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

23.09.2019 22:47, Eric Blake wrote:
> On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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.
>>
>> It also fixes two issues:
>> 1. Fix issue with error_fatal & error_append_hint: user can't see these
>> hints, because exit() happens in error_setg earlier than hint is
>> appended. [Reported by Greg Kurz]
>>
>> 2. Fix issue with 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
> 
> doesn't
> 
>> local_err+error_propagate pattern, which will definitely fix the issue)
>> [Reported by Kevin Wolf]
>>
>> It's still an RFC, due to the following reasons:
>>
>> 1. I'm new to coccinella, so I failed to do the following pattern:
>>
>>   <...
>> - goto out;
>> + return;
>>   ...>
>> - out:
>> - error_propagate(errp, local_err)
>>
>> So, here is compilation fix 08.. Who can help with it? If nobody, 08 is
>> to be merged to 07 by hand.
> 
> I'm not sure either; but I agree that if we can't figure out how to make
> Coccinelle do quite what we want, that we are better off squashing in
> compile fixes.
> 
> Also, while I like Coccinelle for automating the conversion, it's harder
> to get everyone to run it; it would be nice if we could also figure out
> a patch to scripts/checkpatch.pl that for any instance of 'Error
> **errp)\n{\n' not followed by either } or the new macro, we flag that as
> a checkpatch warning or error.
> 
>>
>> 2. Question about using new macro in empty stub functions - see 09
> 
> It would be nice if we could exempt empty functions - no need to use the
> macro if there is no function body otherwise.  I'm not sure if
> Coccinelle can do that filtering during the conversion, or if we clean
> up by hand after the fact.
> 
>>
>> 3. What to do with huge auto-generated commit 07? Should I split it
>> per-maintainer or per-subsystem, or leave it as-is?
> 
> It's big. I'd split it into multiple patches (and reduce the cc - except
> for the cover letter, the rest of the patches can be limited to the
> actual maintainer/subsystem affected rather than everyone involved
> anywhere else in the series. With the current large cc, anyone that
> replies gets several mail bounces about "too many recipients").  It may
> be easier to split along directory boundaries than by maintainer
> boundaries.  Markus has applied large tree-wide Coccinelle cleanups
> before, maybe he has some advice.


If split by subsystem it would be 200+ patches:
git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l
205


Try to look at larger subsystem:
git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l
139

still too many.. Or is it OK?


> 
>>
>> 4. Also, checkpatch has some complains about 07 patch:
>>    - using tabs.. (hmm exactly stubs functions..)
>>    - empty ifs
>>    Again, I don't see any ways to fix it other that by hand and merge to
>>    07..
> 
> Hand cleanups for formatting or compilation fixes to Coccinelle's work
> is not an uncommon issue after large patches; thankfully it's also not
> very difficult (and surprisingly needed in very few places compared to
> how much actually gets touched).
> 
>>
>> ==================
>>
>> Also, if we decide, that this all is too huge, here is plan B:
>>
>> 1. apply 01
>> 2. fix only functions that don't use local_err and use
>> error_append_hint, by just invocation of new macro at function start -
>> it will substitute Greg's series with no pain.
>> 3[optional]. Do full update for some subsystems, for example, only for
>> block* and nbd*
> 
> Even if we go with plan B, it's still worth checking in a Coccinelle
> script that we can periodically run to make sure we aren't missing out
> on the use of the macro where it is needed.
> 
>>
>> Vladimir Sementsov-Ogievskiy (9):
>>    error: auto propagated local_err
>>    qapi/error: add (Error **errp) cleaning APIs
>>    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
>>    scripts: add coccinelle script to use auto propagated errp
>>    Use auto-propagated errp
>>    fix-compilation: empty goto
>>    fix-compilation: includes
>>
>>   include/hw/pci-host/spapr.h                   |   2 +
>>   include/monitor/hmp.h                         |   2 +-
>>   include/qapi/error.h                          |  61 ++++-
>>   target/ppc/kvm_ppc.h                          |   3 +
>>   target/s390x/cpu_models.h                     |   3 +
>>   ui/vnc.h                                      |   2 +-
> 
>>   vl.c                                          |  13 +-
>>   scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
>>   319 files changed, 2729 insertions(+), 4245 deletions(-)
>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
> 
> The diffstat is huge, but promising.
> 


-- 
Best regards,
Vladimir