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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
