Re: [PATCH v11 8/8] xen: introduce ERRP_AUTO_PROPAGATE

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/3/20 11:08 AM, Vladimir Sementsov-Ogievskiy wrote:
>> If we want to add some info to errp (by error_prepend() or
>> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
>> Otherwise, this info will not be added when errp == &error_fatal
>> (the program will exit prior to the error_append_hint() or
>> error_prepend() call).  Fix such cases.
>> 
>> If we want to check error after errp-function call, we need to
>> introduce local_err and then propagate it to errp. Instead, use
>> ERRP_AUTO_PROPAGATE macro, benefits are:
>> 1. No need of explicit error_propagate call
>> 2. No need of explicit local_err variable: use errp directly
>> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
>>    &error_fatal, this means that we don't break error_abort
>>    (we'll abort on error_set, not on error_propagate)
>> 
>> This commit is generated by command
>> 
>>     sed -n '/^X86 Xen CPUs$/,/^$/{s/^F: //p}' MAINTAINERS | \
>>     xargs git ls-files | grep '\.[hc]$' | \
>>     xargs spatch \
>>         --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>         --macro-file scripts/cocci-macro-file.h \
>>         --in-place --no-show-diff --max-width 80
>> 
>> Reported-by: Kevin Wolf <kwolf@redhat.com>
>> Reported-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  hw/block/dataplane/xen-block.c |  17 +++---
>>  hw/block/xen-block.c           | 102 ++++++++++++++-------------------
>>  hw/pci-host/xen_igd_pt.c       |   7 +--
>>  hw/xen/xen-backend.c           |   7 +--
>>  hw/xen/xen-bus.c               |  92 +++++++++++++----------------
>>  hw/xen/xen-host-pci-device.c   |  27 +++++----
>>  hw/xen/xen_pt.c                |  25 ++++----
>>  hw/xen/xen_pt_config_init.c    |  17 +++---
>>  8 files changed, 128 insertions(+), 166 deletions(-)
>
> Without the description, this patch has 800 lines of diff...
> It killed me, I don't have the energy to review patch #7 of this
> series after that, sorry.
> Consider splitting such mechanical patches next time. Here it
> could have been hw/block, hw/pci-host, hw/xen.

Probably my fault; I asked for less fine-grained splitting.

Finding a split of a tree-wide transformation that pleases everyone is
basically impossible.

The conversion to ERRP_AUTO_PROPAGATE() could be one patch per function,
but that would be excessive.

Vladimir chose to split along maintenance boundaries, so he can cc: the
right people on the right code.  I agree with the idea.  The difficulty
is which boundaries.  Our code is not partitioned into maintenance
domains.  Instead, we have overlapping sets.  Makes sense, because it
mirrors how we actually maintain it.

Because of that, a blind split guided by MAINTAINERS won't work well.  A
split that makes sense needs a bit of human judgement, too.

This part makes perfect sense to me from the cc: point of view: it's
Xen, the whole of Xen, and nothing but Xen.

I acknowledge that its size made it exhausting for you to review.  I
didn't expect that, probably because after having spent hours on
reviewing and improving the macro and the Coccinelle script, I know
exactly what to look for, and also consider the script trustworthy[*].

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thank you, much appreciated!


[*] I've learned not to trust Coccinelle 100%, ever.