Re: [PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

01.10.2019 19:50, Eric Blake wrote:
> On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> error_append_hint and error_prepend will not work, if errp ==
>> &fatal_error, as program will exit before error_append_hint or
>> error_prepend call. Fix this by use of special macro
>> ERRP_AUTO_PROPAGATE.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
> 
>> +++ b/scripts/coccinelle/fix-error-add-info.cocci
>> @@ -0,0 +1,28 @@
>> +@rule0@
>> +// Add invocation to errp-functions
>> +identifier fn;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
> 
> When running this script, I get lots of warnings:
> 
> init_defs_builtins: /usr/lib64/coccinelle/standard.h
> warning: line 6: should errp be a metavariable?
> warning: line 11: should errp be a metavariable?
> warning: line 13: should errp be a metavariable?
> HANDLING: include/block/nbd.h
> 
> This means we are trying to patch only an actual variable named 'errp', and not a meta-variable representing any variable of type Error ** regardless of whether it is named errp or something else.  I think that's okay, in part because ERRP_AUTO_PROPAGATE() expects the exact name 'errp' to be in scope, so we cannot apply it to other variable names).  But I still have to wonder if there is a way to silence the warning, or if we have any outliers (other than the intentional Error **errp_in in patch 1) using a different name for a parameter of type Error **, which should also be tweaked.
> 


Hmm, good question:

git grep 'Error \*\*' | grep -v '\<Error \*\*\(errp\|)\)'
block/nbd.c:                                   int ret, Error **local_err)
block/quorum.c:    /* XXX - would be nice if we could pass in the Error **
block/snapshot.c:                             Error **err)
docs/devel/qapi-code-gen.txt:complex argument type.  It takes an additional Error ** argument in
docs/devel/writing-qmp-commands.txt:3. It takes an "Error **" argument. This is required. Later we will see how to
hw/core/qdev.c:static bool check_only_migratable(Object *obj, Error **err)
hw/core/qdev.c:        Error **local_errp = NULL;
hw/core/qdev.c:static bool device_get_hotplugged(Object *obj, Error **err)
hw/i386/amd_iommu.c:static void amdvi_realize(DeviceState *dev, Error **err)
hw/s390x/event-facility.c:static bool sclp_event_get_allow_all_mask_sizes(Object *obj, Error **e)
hw/s390x/s390-stattrib.c:static inline bool s390_stattrib_get_migration_enabled(Object *obj, Error **e)
hw/sd/sdhci.c:static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
hw/tpm/tpm_emulator.c:                                     Error **err)
hw/usb/dev-network.c:static void usb_net_realize(USBDevice *dev, Error **errrp)
include/block/snapshot.h:                             Error **err);
include/io/channel.h:                           Error **erp);
include/qapi/error.h:void error_propagate(Error **dst_errp, Error *local_err);
include/qapi/error.h:void error_propagate_prepend(Error **dst_errp, Error *local_err,
include/qom/object.h:                                    const uint64_t *v, Error **Errp);
include/qom/object.h:                                          const uint64_t *v, Error **Errp);
job.c:/* A wrapper around job_cancel() taking an Error ** parameter so it may be
monitor/qmp-cmds.c:void qmp_system_powerdown(Error **erp)
qga/commands-posix.c:GuestUserList *qmp_guest_get_users(Error **err)
qga/commands-win32.c:GuestUserList *qmp_guest_get_users(Error **err)
qga/commands.c:GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
qga/commands.c:                       Error **err)
qga/commands.c:GuestHostName *qmp_guest_get_host_name(Error **err)
scripts/coccinelle/remove_local_err.cocci: F(..., Error **ERRP)
tests/tpm-util.c:                              SocketAddress **addr, GError **error)
tests/tpm-util.h:                              SocketAddress **addr, GError **error);
util/error.c:void error_propagate(Error **dst_errp, Error *local_err)
util/error.c:void error_propagate_prepend(Error **dst_errp, Error *err,


My favorite is errrp :)

But I tend to not do more effort, until I don't even know, will these series be
applied or not. All these cases are to be checked and fixed by hand, it may be done later.

Check possibly missed by previous grep things:

git grep 'Error \*\*' | grep 'Error \*\*.*Error \*\*'
include/qemu/job.h:int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp);
job.c:int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)

- seems, nothing missed.

-- 
Best regards,
Vladimir