| Message ID | 20191001155319.8066-6-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | error: auto propagated local_err | expand |
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. This patch doesn't actually fix any code, but adds the script to enable automating the fixing of the code in subsequent patches. Tweaking the commit message to make that point clear might be helpful. > scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++++++++++++++ > 1 file changed, 28 insertions(+) > create mode 100644 scripts/coccinelle/fix-error-add-info.cocci > > diff --git a/scripts/coccinelle/fix-error-add-info.cocci b/scripts/coccinelle/fix-error-add-info.cocci > new file mode 100644 > index 0000000000..34fa3be720 > --- /dev/null > +++ b/scripts/coccinelle/fix-error-add-info.cocci > @@ -0,0 +1,28 @@ > +@rule0@ > +// Add invocation to errp-functions > +identifier fn; > +@@ > + > + fn(..., Error **errp, ...) > + { > ++ ERRP_AUTO_PROPAGATE(); > + <+... > +( > + error_append_hint(errp, ...); > +| > + error_prepend(errp, ...); > +) So, for now, you aren't addressing any *errp usage, or any potential cleanups of error_propagate. But that's okay; your cover letter did call out that you were only addressing 1 part out of 3 potential uses just to get some motion, based on the size of the effort. > + ...+> > + } > + > +@@ > +// Drop doubled invocation > +identifier rule0.fn; > +@@ > + > + fn(...) > +{ > + ERRP_AUTO_PROPAGATE(); > +- ERRP_AUTO_PROPAGATE(); > + ... > +} > This looks idempotent once a file is patched, and safe to rerun as many times in the future as needed. I'm still hoping we can find a way to make scripts/checkpatch.pl also do a sanity check, but as it's harder to parse C in perl than in Coccinelle, I can live with just the .cocci script in-tree as long as we remember to manually run it periodically. Reviewed-by: Eric Blake <eblake@redhat.com>
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.
01.10.2019 19:22, 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. > > This patch doesn't actually fix any code, but adds the script to enable automating the fixing of the code in subsequent patches. Tweaking the commit message to make that point clear might be helpful. Hmm, so, maybe, switch "Fix this by use ..." to To fix code with help of this script do spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci FILE_TO_FIX > > >> scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> create mode 100644 scripts/coccinelle/fix-error-add-info.cocci >> >> diff --git a/scripts/coccinelle/fix-error-add-info.cocci b/scripts/coccinelle/fix-error-add-info.cocci >> new file mode 100644 >> index 0000000000..34fa3be720 >> --- /dev/null >> +++ b/scripts/coccinelle/fix-error-add-info.cocci >> @@ -0,0 +1,28 @@ >> +@rule0@ >> +// Add invocation to errp-functions >> +identifier fn; >> +@@ >> + >> + fn(..., Error **errp, ...) >> + { >> ++ ERRP_AUTO_PROPAGATE(); >> + <+... >> +( >> + error_append_hint(errp, ...); >> +| >> + error_prepend(errp, ...); >> +) > > So, for now, you aren't addressing any *errp usage, or any potential cleanups of error_propagate. But that's okay; your cover letter did call out that you were only addressing 1 part out of 3 potential uses just to get some motion, based on the size of the effort. > >> + ...+> >> + } >> + >> +@@ >> +// Drop doubled invocation >> +identifier rule0.fn; >> +@@ >> + >> + fn(...) >> +{ >> + ERRP_AUTO_PROPAGATE(); >> +- ERRP_AUTO_PROPAGATE(); >> + ... >> +} >> > > This looks idempotent once a file is patched, and safe to rerun as many times in the future as needed. I'm still hoping we can find a way to make scripts/checkpatch.pl also do a sanity check, but as it's harder to parse C in perl than in Coccinelle, I can live with just the .cocci script in-tree as long as we remember to manually run it periodically. scripts/checkpatch.pl is so unfriendly.. And to run coccinelle scripts we need working directory, not just patch files. I imagine the following: move scripts/checkpatch.pl to scripts/checkpatch dir Create scripts/checkpatch.py (or python/checkpatch.py, I don't know what is the idea of python/ dir), which will work only on commits or on patch files with specified base (master by default), it will create temporary worktree, checkout corresponding commit and then run scripts from scripts/checkpatch/, at least it would be checkpatch.pl and coccinelle.sh (which will run some coccinelle scripts from coccinelle dir).. > > Reviewed-by: Eric Blake <eblake@redhat.com> >
On 10/1/19 11:50 AM, 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; Adding: symbol errp; to this section appears to be the correct solution for... >> +@@ >> + >> + 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 > silencing these warnings. But it's still not enough to fix the issue I'm pointing out in patch 23 being incomplete.
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.
diff --git a/scripts/coccinelle/fix-error-add-info.cocci b/scripts/coccinelle/fix-error-add-info.cocci new file mode 100644 index 0000000000..34fa3be720 --- /dev/null +++ b/scripts/coccinelle/fix-error-add-info.cocci @@ -0,0 +1,28 @@ +@rule0@ +// Add invocation to errp-functions +identifier fn; +@@ + + fn(..., Error **errp, ...) + { ++ ERRP_AUTO_PROPAGATE(); + <+... +( + error_append_hint(errp, ...); +| + error_prepend(errp, ...); +) + ...+> + } + +@@ +// Drop doubled invocation +identifier rule0.fn; +@@ + + fn(...) +{ + ERRP_AUTO_PROPAGATE(); +- ERRP_AUTO_PROPAGATE(); + ... +}
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> --- CC: kwolf@redhat.com CC: mreitz@redhat.com CC: jsnow@redhat.com CC: fam@euphon.net CC: sw@weilnetz.de CC: codyprime@gmail.com CC: marcandre.lureau@redhat.com CC: pbonzini@redhat.com CC: groug@kaod.org CC: sundeep.lkml@gmail.com CC: peter.maydell@linaro.org CC: stefanha@redhat.com CC: pburton@wavecomp.com CC: arikalo@wavecomp.com CC: berrange@redhat.com CC: ehabkost@redhat.com CC: david@gibson.dropbear.id.au CC: clg@kaod.org CC: mst@redhat.com CC: marcel.apfelbaum@gmail.com CC: mark.cave-ayland@ilande.co.uk CC: yuval.shaia@oracle.com CC: cohuck@redhat.com CC: farman@linux.ibm.com CC: rth@twiddle.net CC: david@redhat.com CC: pasic@linux.ibm.com CC: borntraeger@de.ibm.com CC: kraxel@redhat.com CC: alex.williamson@redhat.com CC: andrew@aj.id.au CC: joel@jms.id.au CC: eblake@redhat.com CC: armbru@redhat.com CC: mdroth@linux.vnet.ibm.com CC: quintela@redhat.com CC: dgilbert@redhat.com CC: jasowang@redhat.com CC: qemu-block@nongnu.org CC: integration@gluster.org CC: qemu-arm@nongnu.org CC: qemu-ppc@nongnu.org CC: qemu-s390x@nongnu.org scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 scripts/coccinelle/fix-error-add-info.cocci