Re: [PATCH-for-5.1 v3 1/7] scripts/coccinelle: Use &error_abort in TypeInfo::instance_init()

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> writes:

> The instance_init() calls are not suppose to fail. Add a
> Coccinelle script to use &error_abort instead of ignoring
> errors by using a NULL Error*.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> v3: Improved script (Vladimir Sementsov-Ogievskiy suggestions)
>
>  .../use-error_abort-in-instance_init.cocci    | 45 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 46 insertions(+)
>  create mode 100644 scripts/coccinelle/use-error_abort-in-instance_init.cocci
>
> diff --git a/scripts/coccinelle/use-error_abort-in-instance_init.cocci 
> b/scripts/coccinelle/use-error_abort-in-instance_init.cocci
> new file mode 100644
> index 0000000000..706c60163c
> --- /dev/null
> +++ b/scripts/coccinelle/use-error_abort-in-instance_init.cocci
> @@ -0,0 +1,45 @@
> +// Use &error_abort in TypeInfo::instance_init()
> +//
> +// 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 --in-place
> +//
> +// Inspired by https://www.mail-archive.com/address@hidden/msg692500.html
> +// and https://www.mail-archive.com/address@hidden/msg693637.html
> +
> +
> +@ has_qapi_error @
> +@@
> +    #include "qapi/error.h"
> +
> +
> +@ match_instance_init @
> +TypeInfo info;
> +identifier instance_initfn;
> +@@
> +    info.instance_init = instance_initfn;
> +
> +
> +@ use_error_abort_in_instance_init @
> +identifier match_instance_init.instance_initfn;
> +identifier func_with_error != {qbus_create_inplace, object_initialize_child};
> +position pos;
> +@@
> +void instance_initfn(...)
> +{
> +   <+...
> +   func_with_error@pos(...,
> +-                           NULL);
> ++                           &error_abort);
> +   ...+>
> +}

While I expect the function can't actually fail for most instances of
this pattern, I can't exclude the possibility of "can't fail, but we're
not intested in the Error object".  The transformation is an improvement
in the former case, but incorrect in the latter case.  Patches need
extra-careful review.  Please explain that in both the script and the
commit message.

> +
> +
> +@ depends on use_error_abort_in_instance_init && !has_qapi_error @
> +@@
> +    #include ...
> ++   #include "qapi/error.h"
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f996e72780..77b93612bc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2060,6 +2060,7 @@ F: scripts/coccinelle/error-use-after-free.cocci
>  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_abort-in-instance_init.cocci
>  F: scripts/coccinelle/use-error_fatal.cocci
>  F: scripts/coccinelle/use-error_propagate-in-realize.cocci