Philippe Mathieu-Daudé <f4...@amsat.org> 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 <peter.mayd...@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> ---
> .../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/qemu-devel@nongnu.org/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 <alex.ben...@linaro.org>
