Re: [PATCH-for-5.0 00/12] hw: Add missing error-propagation code

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Hi Markus, Peter.
>
> On 3/31/20 3:23 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>
>>> This series is inspired of Peter fix:
>>> "hw/arm/xlnx-zynqmp.c: fix some error-handling code"
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg691636.html
>>>
>>> Add a cocci script to fix the other places.
>>>
>>> Based-on: <20200324134947.15384-1-peter.maydell@linaro.org>
>>
>> I skimmed the code patches [PATCH 02-12/12], and they look like bug
>> fixes.  Other reviewers raised a few issues.
>>
>> I also skimmed the Coccinelle script [PATCH 01].  Peter pointed out a
>> few things it apparently missed (e.g. in review of PATCH 06+11).
>> Moreover, the bug pattern applies beyond object_property_set() &
>> friends.  Perhaps the script can be generalized.  No reason to hold
>> fixes.  We may want to add suitable notes to the scipt, though.
>>
>> Can you address the reviews in a v2, so we can get the fixes into -rc1,
>> due today?
>
> Status on this series (sorry I didn't update earlier).
>
> I addressed Peter's comments, improved/simplified/documented the cocci
> script (which I split in smaller ones).
>
> Peter suggested other functions can be checked too, not only the
> "^object_property_set_.*" matches. Indeed, more patches added. Some
> are big.
>
> Another suggestion is replace in init() 'NULL' Error* final argument
> by &error_abort. This can be another series on top.
> However I noticed we can reduce the error_propagate() generated calls
> in many places, when both init()/realize() exist and the property set
> is not dependent of parent operation, by moving these calls from
> realize() to init(). Another cocci script. But to make sense it has to
> be run previous the "add missing error_propagate" one.
>
> While writing the cocci patches, I had 3 different Coccinelle failures.
> Failures not due to a spatch bug, but timeout because C source hard to
> process. Indeed the C source code was dubious, could get some
> simplification rewrite. Then spatch could transform them. More patches
> in the middle.
>
> Now I'm at 47 patches, the reviewed patches at the end of the series.
> Too much for RC2. Since I don't think these are critical bugs, but
> improvements, are you OK to postpone this series to 5.1?

Punting bug fixes to a later release is always kind of sad, but getting
that many patches reviewed properly in time for -rc2 feels hopeless, and
the bugs old and unthreatending on first glance.

> If you think a patch deserves to be in 5.0, point me at it and I can
> send it ASAP with comments addressed. Else I'll post my series as
> -for-5.1 soon.

I'm okay with punting the whole series to 5.1.

We have quite some Error work pending for 5.1.  Beware of conflicts.

In particular, I now plan to make object_property_set() & friends return
a useful value.  This should make your patches a bit smaller.