[sheepdog] [RFC v5 000/126] error: auto propagated local_err

[Markus Armbruster]

Please accept my sincere apologies for taking so long to reply.  A few
thoughts before I dig deeper.

Vladimir Sementsov-Ogievskiy <vsementsov at virtuozzo.com> writes:

> Hi all!
>
> At the request of Markus: full version of errp propagation. Let's look
> at it. Cover as much as possible, except inserting macro invocation
> where it's not necessary.
>
> It's huge, and so it's an RFC.

It's a monster.  Best to get it into full view before we commit to
fighting it.

> In v5 I've added a lot more preparation cleanups:
> 01-23 are preparation cleanups
>   01: not changed, keep Eric's r-b
>   02: improve commit msg [Markus], keep Eric's r-b
>   03: changed, only error API here, drop r-b
> 24 is core macro
>   - improve cover letter, wording and macro code style
>   - keep Eric's r-b
> 25-26: automation scripts
>    - commit-per-subsystem changed a lot. it's a draft, don't bother too
>      much with it
>    - coccinelle: add support of error_propagate_prepend
>
> 27-126: generated patches

Splitting up the monster can make fighting it easier.

Your description suggests three high-level parts:

Part 1: Preparation (makes sense by itself)
Part 2: Error interface update (with rules what code should do now)
Part 3: Make the code obey the new rules everywhere

I hope we can get part 1 out of the way quickly.  Diffstat:

 backends/cryptodev.c       |  11 +---
 block/nbd.c                |  10 +--
 block/snapshot.c           |   4 +-
 dump/dump-hmp-cmds.c       |   4 +-
 hw/9pfs/9p-local.c         |   4 +-
 hw/9pfs/9p-proxy.c         |   5 +-
 hw/core/loader-fit.c       |   5 +-
 hw/core/machine-hmp-cmds.c |   6 +-
 hw/core/qdev.c             |  28 ++++----
 hw/i386/amd_iommu.c        |  14 ++--
 hw/ppc/spapr.c             |   2 +-
 hw/s390x/event-facility.c  |   2 +-
 hw/s390x/s390-stattrib.c   |   3 +-
 hw/sd/sdhci.c              |   2 +-
 hw/tpm/tpm_emulator.c      |   8 +--
 hw/usb/dev-network.c       |   2 +-
 hw/vfio/ap.c               |  16 +----
 include/block/snapshot.h   |   2 +-
 include/monitor/hmp.h      |   2 +-
 include/qapi/error.h       |  69 ++++++++++++++++++--
 include/qom/object.h       |   4 +-
 monitor/hmp-cmds.c         | 155 ++++++++++++++++++++++-----------------------
 monitor/qmp-cmds.c         |   2 +-
 net/net.c                  |  17 ++---
 qdev-monitor.c             |  28 ++++----
 qga/commands-posix.c       |   2 +-
 qga/commands-win32.c       |   2 +-
 qga/commands.c             |  12 ++--
 qom/qom-hmp-cmds.c         |   4 +-
 target/ppc/kvm.c           |   6 +-
 target/ppc/kvm_ppc.h       |   4 +-
 ui/vnc.c                   |  20 ++----
 ui/vnc.h                   |   2 +-
 util/error.c               |  30 ++++-----
 34 files changed, 261 insertions(+), 226 deletions(-)

At first glance, I can see bug fixes, non-mechanical cleanups, and
mechanical cleanups.

Within each of these three groups, we have related sub-groups.  For
instance, several patches clean up funny names for the common Error **
parameters.  Several more rename "uncommon" Error ** parameters, to
signal their uncommon role.  I doubt splitting up these subgroups of
related mechanical changes along subsystem lines is worthwhile.

Part 2 needs careful interface review.  Having part 3 ready helps there,
because we can see rather than guess how the interface changes play out.
We really want to get this part right from the start, because if we
don't, we get to do part 3 again.

Part 3 is what makes this a monster.  I understand it's mechanical.  We
can merge it incrementally, but we do want to merge it all, and sooner
rather than later, to avoid a mix of old and new error handling code.
Such mixes inevitably confuse developers, and lead to new instances of
the old patterns creeping in.

I do have doubts about your automated split.

I acknowledge maintainers of active subsystems may want to merge this on
their own terms, to minimize disruption.  Splitting off sub-monsters for
them makes sense.  Splitting off the long tail of less busy subsystems
not so much; it'll only drag out the merging.  Your list below shows 100
parts, and chasing their maintainers is not going to be a fun
experience.

Moreover, using MAINTAINERS to guide an automatic split is a cute idea,
but it falls apart when MAINTAINERS attributes the same file to several
subsystems, which is fairly common.  A sane split requires human touch.

Instead, I'd start with big subsystems with maintainers known to be
sympathetic to this effort.  Split off their sub-monsters, get them
merged.  Iterate until the remainder can be merged in one final push.

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

More cleverness, less code, avoids one kind of error (forgetting manual
propagate when we should), risks another kind of error (automatic
propagate when we shouldn't).  Tradeoffs, but the general feeling among
reviewers appears to be positive.

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

Yes, broken by design, hurts users.

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

Yes, broken by design, inconveniences developers.

> ====
>
> Generated patches split:
>
> misc
>    hw/misc/ivshmem.c
>    hw/misc/tmp105.c
>    hw/misc/tmp421.c
[99 more...]