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

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

28.11.2019 11:54, Markus Armbruster wrote:
> Please accept my sincere apologies for taking so long to reply.  A few
> thoughts before I dig deeper.
> 
> Vladimir Sementsov-Ogievskiy <vsementsov@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)

I already resent part 1 all patches (handling review comments) in separate as v6.
If it is convenient, I can resend them in one series as v7.

> Part 2: Error interface update (with rules what code should do now)

Note, that patch 21 is actually from part2, not part1.
So Part 2 is 21, 24, 25.
So I wait for your comments and resend (if needed) as separate small series.

And 26 is auto-patch-splitter, but we don't need it now, if we are going
to start from several big subsystems.

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

Do you mean to send them as separate per-subsystem series, or all in one,
but limited to some subsystems?

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

Thanks!

-- 
Best regards,
Vladimir