Markus Armbruster <arm...@redhat.com> writes:
> 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().
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.
