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

[Markus Armbruster]

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

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

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.

> ++   }
> +    ...+>
> +}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6203543ec0..54e05ecbdf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2060,6 +2060,7 @@ F: scripts/coccinelle/error_propagate_null.cocci
>  F: scripts/coccinelle/remove_local_err.cocci
>  F: scripts/coccinelle/simplify-init-realize-error_propagate.cocci
>  F: scripts/coccinelle/use-error_fatal.cocci
> +F: scripts/coccinelle/use-error_propagate-in-realize.cocci
>  
>  GDB stub
>  M: Alex Bennée <address@hidden>