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

[Markus Armbruster]

Eduardo Habkost  writes:

> On Mon, Jun 13, 2016 at 08:49:37PM +0200, Markus Armbruster wrote:
>> Eric Blake  writes:
> [...]
>> >> 
>> >> 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, _err);
>> >> acquire_privilege(SE_SHUTDOWN_NAME, _err);
>> >> execute_async(do_suspend, mode, _err);
>> >
>> > That usage is a bug.  A Coccinelle script to root out such buggy
>> > instances would be nice.
>> 
>> We've discussed this before.  See for instance commit 297a364:
>> 
>> qapi: Replace uncommon use of the error API by the common one
>
> That explains why I was confused: I remember seeing that QAPI
> visitors could be called with *errp set.

Nothing confuses as effectively as bad examples.

> I will try to change the script as suggested, to only apply the
> changes if errp is never touched before the error_setg() call.

Thanks!

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

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 08:49:37PM +0200, Markus Armbruster wrote:
> Eric Blake  writes:
[...]
> >> 
> >> 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, _err);
> >> acquire_privilege(SE_SHUTDOWN_NAME, _err);
> >> execute_async(do_suspend, mode, _err);
> >
> > That usage is a bug.  A Coccinelle script to root out such buggy
> > instances would be nice.
> 
> We've discussed this before.  See for instance commit 297a364:
> 
> qapi: Replace uncommon use of the error API by the common one

That explains why I was confused: I remember seeing that QAPI
visitors could be called with *errp set.

I will try to change the script as suggested, to only apply the
changes if errp is never touched before the error_setg() call.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 10:01:16AM -0600, Eric Blake wrote:
> On 06/13/2016 09:52 AM, Eduardo Habkost wrote:
[...]
> > 
> > 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, _err);
> > acquire_privilege(SE_SHUTDOWN_NAME, _err);
> > execute_async(do_suspend, mode, _err);
> 
> That usage is a bug.  A Coccinelle script to root out such buggy
> instances would be nice.

I've tried to write one, but got lots of false positives due to
error-checking using the function return value, not local_err.
For reference, this is the script I have used:

@@
typedef Error;
identifier F1 !~ "hmp_handle_error|error_free_or_abort";
identifier F2 !~ "hmp_handle_error|error_free_or_abort";
idexpression Error *ERR;
@@
-F1(..., )
+FIXME_incorrect_error_usage1()
... when != ERR
-F2(..., )
+FIXME_incorrect_error_usage2()

@@
identifier F1 !~ "hmp_handle_error|error_free_or_abort";
identifier F2 !~ "hmp_handle_error|error_free_or_abort";
idexpression Error **ERRP;
@@
-F1(..., ERRP)
+FIXME_incorrect_error_usage1()
 ... when != ERRP
-F2(..., ERRP)
+FIXME_incorrect_error_usage2()

-- 
Eduardo

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

[Markus Armbruster]

Eric Blake  writes:

> On 06/13/2016 09:52 AM, Eduardo Habkost wrote:
>
>>>
>>> There is an (ugly) difference between
>>>
>>> error_setg(_err, ...);
>>> error_propagate(errp, _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?
>
> Maybe, but I think it weakens our position. An abort() on an attempt to
> incorrectly set an error twice helps catch errors where we are throwing
> away a more useful first error message.  The documentation for
> error_propagate() already mentioned that it was an exception to the rule.

For what it's worth, both g_set_error(err, ...) and
g_propagate_error(err, ...) require !err || !*err.  To accumulate
multiple errors so that the first one wins, you have to do something
like

if (local_err) {
g_clear_error(errp);
g_propagate_error(errp, local_err);
}

error.h happend before we got GLib.  Its designers chose to deviate from
GLib and made error_propagate() accumulate errors.  This trades the
ability to detect misuse for less boilerplate:

error_propagate(errp, local_err);

Tightening error_propagate() now would lead to a rather tedious hunt for
callers that rely on its accumulating behavior.

We could do it incrementally by renaming error_propagate() to
error_accumulate(), and have a new error_propagate() that's consistent
with error_setg().  Not sure it's worth it.

Loosening error_setg() & friends is also possible, but we'd detect fewer
misuse, and we'd be left with some superfluous code to clean up.  Not
sure that's smart.

>>> 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?
>
> Except for error_propagate(), no.

By convention, all functions taking an Error **errp argument expect one
that can be safely passed to error_setg().  That means !errp || errp ==
_abort || errp == _fatal || !*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, _err);
>> acquire_privilege(SE_SHUTDOWN_NAME, _err);
>> execute_async(do_suspend, mode, _err);
>
> That usage is a bug.  A Coccinelle script to root out such buggy
> instances would be nice.

We've discussed this before.  See for instance commit 297a364:

qapi: Replace uncommon use of the error API by the common one

We commonly use the error API like this:

err = NULL;
foo(..., );
if (err) {
goto out;
}
bar(..., );

Every error source is checked separately.  The second function is only
called when the first one succeeds.  Both functions are free to pass
their argument to error_set().  Because error_set() asserts no error
has been set, this effectively means they must not be called with an
error set.

The qapi-generated code uses the error API differently:

// *errp was initialized to NULL somewhere up the call chain
frob(..., errp);
gnat(..., errp);

Errors accumulate in *errp: first error wins, subsequent errors get
dropped.  To make this work, the second function does nothing when
called with an error set.  Requires non-null errp, or else the second
function can't see the first one fail.

This usage has also bled into visitor tests, and two device model
object property getters rtc_get_date() and balloon_stats_get_all().

With the "accumulate" technique, you need fewer error checks in
callers, and buy that with an error check in every callee.  Can be
nice.

However, mixing the two techniques is confusing.  You can't use the
"accumulate" technique with functions designed for the "check
separately" technique.  You can use the "check separately" technique
with functions designed for the "accumulate" technique, but then
error_set() can't catch you setting an error more than once.

Standardize on the "check separately" technique for now, because it's
overwhelmingly prevalent.

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

[Eric Blake]

On 06/13/2016 09:52 AM, Eduardo Habkost wrote:

>>
>> There is an (ugly) difference between
>>
>> error_setg(_err, ...);
>> error_propagate(errp, _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?

Maybe, but I think it weakens our position. An abort() on an attempt to
incorrectly set an error twice helps catch errors where we are throwing
away a more useful first error message.  The documentation for
error_propagate() already mentioned that it was an exception to the rule.

> 
>>
>> 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?

Except for error_propagate(), no.

> 
> 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, _err);
> acquire_privilege(SE_SHUTDOWN_NAME, _err);
> execute_async(do_suspend, mode, _err);

That usage is a bug.  A Coccinelle script to root out such buggy
instances would be nice.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [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  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 
> [...]
> > diff --git a/scripts/coccinelle/remove_local_err.cocci 
> > b/scripts/coccinelle/remove_local_err.cocci
> > new file mode 100644
> > index 000..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, _ERR);
> > +-error_propagate(ERRP, LOCAL_ERR);
> > ++F2(ARGS, ERRP);
> > +|
> > +-V = F2(ARGS, _ERR);
> > +-error_propagate(ERRP, LOCAL_ERR);
> > ++V = F2(ARGS, ERRP);
> > +)
> > + ... when != LOCAL_ERR
> > + }
> 
> There is an (ugly) difference between
> 
> error_setg(_err, ...);
> error_propagate(errp, _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, _err);
acquire_privilege(SE_SHUTDOWN_NAME, _err);
execute_async(do_suspend, mode, _err);

if (local_err) {
error_propagate(errp, local_err);
g_free(mode);
}
}


-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost  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 
[...]
> diff --git a/scripts/coccinelle/remove_local_err.cocci 
> b/scripts/coccinelle/remove_local_err.cocci
> new file mode 100644
> index 000..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, _ERR);
> +-error_propagate(ERRP, LOCAL_ERR);
> ++F2(ARGS, ERRP);
> +|
> +-V = F2(ARGS, _ERR);
> +-error_propagate(ERRP, LOCAL_ERR);
> ++V = F2(ARGS, ERRP);
> +)
> + ... when != LOCAL_ERR
> + }

There is an (ugly) difference between

error_setg(_err, ...);
error_propagate(errp, _err);

and

error_setg(errp, ...);

The latter aborts when @errp already contains an error, the former does
nothing.

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?

[...]