On Mon, Jun 13, 2016 at 01:42:15PM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabk...@redhat.com> writes: > > > This patch simplifies code that uses a local_err variable just to > > immediately use it for an error_propagate() call. > > > > Coccinelle patch used to perform the changes added to > > scripts/coccinelle/remove_local_err.cocci. > > > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> > [...] > > diff --git a/scripts/coccinelle/remove_local_err.cocci > > b/scripts/coccinelle/remove_local_err.cocci > > new file mode 100644 > > index 0000000..5f0b6c0 > > --- /dev/null > > +++ b/scripts/coccinelle/remove_local_err.cocci > > @@ -0,0 +1,27 @@ > > +// Replace unnecessary usage of local_err variable with > > +// direct usage of errp argument > > + > > +@@ > > +expression list ARGS; > > +expression F2; > > +identifier LOCAL_ERR; > > +expression ERRP; > > +idexpression V; > > +typedef Error; > > +expression I; > > I isn't used. > > > +@@ > > + { > > + ... > > +- Error *LOCAL_ERR; > > + ... when != LOCAL_ERR > > +( > > +- F2(ARGS, &LOCAL_ERR); > > +- error_propagate(ERRP, LOCAL_ERR); > > ++ F2(ARGS, ERRP); > > +| > > +- V = F2(ARGS, &LOCAL_ERR); > > +- error_propagate(ERRP, LOCAL_ERR); > > ++ V = F2(ARGS, ERRP); > > +) > > + ... when != LOCAL_ERR > > + } > > There is an (ugly) difference between > > error_setg(&local_err, ...); > error_propagate(errp, &local_err); > > and > > error_setg(errp, ...); > > The latter aborts when @errp already contains an error, the former does > nothing.
Why the difference? Couldn't we change that so that both are equivalent? > > Your transformation has the error_setg() or similar hidden in F2(). It > can add aborts. > > I think it can be salvaged: we know that @errp must not contain an error > on function entry. If @errp doesn't occur elsewhere in this function, > it cannot pick up an error on the way to the transformed spot. Can you > add that to your when constraints? Do we really know that *errp is NULL on entry? Aren't we allowed to call functions with a non-NULL *errp? See, e.g.: void qmp_guest_suspend_disk(Error **errp) { Error *local_err = NULL; GuestSuspendMode *mode = g_new(GuestSuspendMode, 1); *mode = GUEST_SUSPEND_MODE_DISK; check_suspend_mode(*mode, &local_err); acquire_privilege(SE_SHUTDOWN_NAME, &local_err); execute_async(do_suspend, mode, &local_err); if (local_err) { error_propagate(errp, local_err); g_free(mode); } } -- Eduardo