[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC v5 000/126] error: auto propagated local_err
Re: [RFC v5 000/126] error: auto propagated local_err
Thu, 28 Nov 2019 09:20:01 +0000
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 <address@hidden> 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
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
> 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:
> [99 more...]