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

[Eric Blake]

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.

> 
> 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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org