Re: [PATCH-for-5.1 v3 01/23] scripts/coccinelle: Catch missing error_propagate() calls in realize()

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Philippe Mathieu-Daudé <address@hidden> writes:
>
>> In some DeviceClass::realize() while we can propagate errors
>> to the caller, we forgot to do so. Add a Coccinelle patch to
>> automatically add the missing code.
>>
>> Inspired-by: Peter Maydell <address@hidden>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>>  .../use-error_propagate-in-realize.cocci      | 54 +++++++++++++++++++
>>  MAINTAINERS                                   |  1 +
>>  2 files changed, 55 insertions(+)
>>  create mode 100644 scripts/coccinelle/use-error_propagate-in-realize.cocci
>>
>> diff --git a/scripts/coccinelle/use-error_propagate-in-realize.cocci 
>> b/scripts/coccinelle/use-error_propagate-in-realize.cocci
>> new file mode 100644
>> index 0000000000..7b59277a50
>> --- /dev/null
>> +++ b/scripts/coccinelle/use-error_propagate-in-realize.cocci
>> @@ -0,0 +1,54 @@
>> +// Add missing error-propagation code in DeviceClass::realize()
>> +//
>> +// Copyright: (C) 2020 Philippe Mathieu-Daudé
>> +// This work is licensed under the terms of the GNU GPLv2 or later.
>> +//
>> +// spatch \
>> +//  --macro-file scripts/cocci-macro-file.h --include-headers \
>> +//  --sp-file scripts/coccinelle/use-error_abort-in-instance_init.cocci \
>> +//  --keep-comments --timeout 60 --in-place
>> +//
>> +// Inspired by https://www.mail-archive.com/address@hidden/msg691638.html
>> +
>> +
>> +@ match_class_init @
>> +TypeInfo info;
>> +identifier class_initfn;
>> +@@
>> +    info.class_init = class_initfn;
>> +
>> +
>> +@ match_realize @
>> +identifier match_class_init.class_initfn;
>> +DeviceClass *dc;
>> +identifier realizefn;
>> +@@
>> +void class_initfn(...)
>> +{
>> +    ...
>> +    dc->realize = realizefn;
>> +    ...
>> +}
>> +
>> +
>> +@ propagate_in_realize @
>> +identifier match_realize.realizefn;
>> +identifier err;
>> +identifier errp;
>> +identifier func_with_errp;
>> +symbol error_abort, error_fatal;
>> +@@
>> +void realizefn(..., Error **errp)
>> +{
>> +    ...
>> +    Error *err = NULL;
>> +    <+...
>> +    func_with_errp(...,
>> +-                      \(&error_abort\|&error_fatal\));
>> ++                      &err);
>> ++   if (err) {
>> ++       error_propagate(errp, err);
>> ++       return;
>
> Issues:
>
> 0. The script patches only realize() methods of DeviceClass, not of
>    subclasses, and no helpers.
>
>    Example of a subclass method: see my review of "[PATCH-for-5.1 v3
>    02/24] scripts/coccinelle: Script to simplify DeviceClass error
>    propagation".
>
>    Example of a helper method: pnv_chip_core_realize().

One more:

  * The script doesn't patch calls that pass NULL for errp.  The ones
    that can't fail should pass &error_abort instad.  The ones that can
    fail commonly need to error_propagate().  There may be exceptions,
    though.

>
> 1. When the function can't actually fail, the script adds dead error
>    handling.

     &error_abort is not wrong in such cases.

     &error_fatal is always wrong in realize().

> 2. When the function can fail, it may well add incomplete error
>    handling: it can't add the code needed to revert what has been done
>    so far.
>
>    Example: ivshmem_common_realize() has:
>
>         error_setg(&s->migration_blocker,
>                    "Migration is disabled when using feature 'peer mode' in 
> device 'ivshmem'");
>         migrate_add_blocker(s->migration_blocker, &err);
>         if (err) {
>             error_propagate(errp, err);
>             error_free(s->migration_blocker);
>             return;
>         }
>
>    If the error handling was missing, the script would fail to add the
>    error_free().
>
> Even imperfect scripts can lead us to code in need of improvement.  But
> you should explain the script's limitations, both in the script and the
> commit message.