Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 01:42:15PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> [...]
> > 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