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

[Markus Armbruster <armbru@redhat.com>]

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

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

Recommend to await review.  The more we can merge without another
respin, the better.

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

Thanks for the heads-up.

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

Let's make it as easy as we can both for the subsystem maintainers and
for the people trying to track all of it.

When a subsystem takes multiple patches, I'd consider an independent
series to save the maintainer the trouble of extracting multiple patches
from a larger series.

For the ones that take just one patch, I'd consider an omnibus series.
Extracting a single patch is no harder than applying a series, but
tracking one omnibus is easier than a dozen lone patches.

There's no clear line between "busy" and "less busy" subsystem.  Just
start with some obviously busy ones, then iterate.  Each iteration
should be large enough to be worth the overhead, yet small enough not to
scare off reviewers :)

Trust your judgement!

[...]