Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
Peter Crosthwaite peter.crosthwa...@xilinx.com writes:
On Fri, Apr 11, 2014 at 11:41 PM, Markus Armbruster arm...@redhat.com wrote:
Peter Crosthwaite peter.crosthwa...@xilinx.com writes:
On Thu, Apr 10, 2014 at 1:48 AM, Markus Armbruster arm...@redhat.com
wrote:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
That should be the callers problem. If you pass a NULL errp then the
intended semantic is to ignore errors.
The *caller* isn't interested in an error. But the callee's behavior
should *not* be affected by that at all other than not returning an
error.
In particular, the callee should never continue after an error just
because the caller isn't interested in detailed error information.
But the error is from a previous call not the current call. Callers
job to inform second call that first one failed (or in current status
quo - not call the second call at all). But its caller job to know the
dependancy otherwise calls are not self contained.
That's why if (error_is_set(errp)) bail and similar are always wrong
when errp is a parameter that may be null.
Agreed. Don't see the problem here though - it's bad caller code too.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
And this is irregular in that you are now mandating the Error ** and
thus removing the capability to ignore errors.
It is irregular. The irregularity is necessary as long as the function
isn't prepared for a null errp.
My understanding is all functions should be prepared for NULL errp.
If you combine do nothing when called with an error set semantics with
a pass null to ignore errors convention, you set yourself up for
accidental misuse. Consider
visit_type_str(v, foo, foo, errp);
visit_type_str(v, bar, bar, errp);
where visit_type_str() does nothing when error_is_set(errp). This
*breaks* when errp is null, but seeing that takes a non-local argument.
Here's a simple safety rule: if you do anything with an Error * but pass
it on, you must ensure it's not null, and you should make its
non-null-ness locally obvious whenever practical.
This rule should ensure that you can pass null to ignore errors without
changing behavior beyond ignoring the error.
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
I think this code as-is is a good example of what we should do
elsewhere. The code base has bloated with the if (error) { bail; } on
every Error ** accepting API call. I proposed a while back a semantic
that Error ** Accepting APIs perform no action when the error is
already set to allow for long sequences of calls to run without the
constant checks. You then report the first error in a
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
Dr. David Alan Gilbert dgilb...@redhat.com writes:
* Markus Armbruster (arm...@redhat.com) wrote:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
Right, one of the problems is you just have long strings of visit_* calls
and adding a check to each one hides what you're actually doing in a sea
of checks. The downside is that if one of those visit's fails then you've
got no chance of figuring out which one it was.
In my BER world I've got some macros along the lines of:
#define LOCAL_ERR_REPORT(fallout) \
if (local_err) { \
fallout \
}
and at least then I can do things like:
visit_type_str(v, foo, foo, err);
LOCAL_ERR_REPORT( goto out; )
visit_type_str(v, bar, bar, err);
LOCAL_ERR_REPORT( goto out; )
which while not nice,
Understatement :)
means that you can actually follow the code, and
I can also add a printf to the macro to record the function/line so
that when one of them fails I can see which visit was the cause of the problem
(something that's currently very difficult).
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
The other problem is I had a tendency to typo some of the cases to
if (*err) and it's quite hard to spot and you wonder what's going on.
The only help I can offer with that is naming conventions: use errp
only for Error ** variables, and err only for Error *.
I have patches in my queue to clean up current usage.
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
Kevin Wolf kw...@redhat.com writes:
Am 09.04.2014 um 17:48 hat Markus Armbruster geschrieben:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
I agree, use the same style as everywhere else.
The pattern in the generated visitor that I find more annoying, though,
is that it has a lot of code like:
if (!error_is_set(errp)) {
/* long block of code here */
}
And I believe there are even cases where this nests.
I also find if (error) bail_out generally more readable than if
(!error) do_more_work. More so when nested.
I'll see what I can do about it in the generator scripts.
There are also
error_propagate() calls that can (and do in the common case) propagate
NULL, this way selecting the first error, if any, but not stopping on
the first error. I always found it confusing to read that code.
Can you point me to an instance in the generated code?
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
Anthony Liguori anth...@codemonkey.ws writes: The original visiting code was loosely based on ASN1 marshaling code from Samba which used the if error, bail out at the top style of error handling. As use of Error has evolved in QEMU, I agree that the paradigm of bail out as soon as you see an error and fail fast is better so I'd vote for changing the generated code to do that. Okay, I'll give it a try. Thanks!
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
* Markus Armbruster (arm...@redhat.com) wrote:
Dr. David Alan Gilbert dgilb...@redhat.com writes:
* Markus Armbruster (arm...@redhat.com) wrote:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
Right, one of the problems is you just have long strings of visit_* calls
and adding a check to each one hides what you're actually doing in a sea
of checks. The downside is that if one of those visit's fails then you've
got no chance of figuring out which one it was.
In my BER world I've got some macros along the lines of:
#define LOCAL_ERR_REPORT(fallout) \
if (local_err) { \
fallout \
}
and at least then I can do things like:
visit_type_str(v, foo, foo, err);
LOCAL_ERR_REPORT( goto out; )
visit_type_str(v, bar, bar, err);
LOCAL_ERR_REPORT( goto out; )
which while not nice,
Understatement :)
I await the suggestion on how to do it in a nicer way - the
problem is I'd really like to be able to capture which element failed
to be read when reading in a stream, and that's quite difficult if you only
check the 'err' in a few places (yes you can do it by names passed into the
visitors etc but it gets equally messy).
means that you can actually follow the code, and
I can also add a printf to the macro to record the function/line so
that when one of them fails I can see which visit was the cause of the
problem
(something that's currently very difficult).
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
The other problem is I had a tendency to typo some of the cases to
if (*err) and it's quite hard to spot and you wonder what's going on.
The only help I can offer with that is naming conventions: use errp
only for Error ** variables, and err only for Error *.
I have patches in my queue to clean up current usage.
It's in some way why I liked the error_is_set; you ended up with a type
check and it meant you just couldn't make that error.
I did wonder about a modified error_propagate - i.e.
bool error_propagate(Error **dst_err, Error *local_err)
then you do:
if (error_propagate(errp, local_err)) {
goto out;
}
where the error_propagate would do just what it does at the moment, but return
true if local_err had an error, or if errp was non-null and had an error.
error_propagate could be modified to return that bool without changing any
current caller.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
Am 11.04.2014 um 10:28 hat Markus Armbruster geschrieben:
Kevin Wolf kw...@redhat.com writes:
Am 09.04.2014 um 17:48 hat Markus Armbruster geschrieben:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
I agree, use the same style as everywhere else.
The pattern in the generated visitor that I find more annoying, though,
is that it has a lot of code like:
if (!error_is_set(errp)) {
/* long block of code here */
}
And I believe there are even cases where this nests.
I also find if (error) bail_out generally more readable than if
(!error) do_more_work. More so when nested.
I'll see what I can do about it in the generator scripts.
There are also
error_propagate() calls that can (and do in the common case) propagate
NULL, this way selecting the first error, if any, but not stopping on
the first error. I always found it confusing to read that code.
Can you point me to an instance in the generated code?
It's more or less everywhere. The pattern for structs is something like
this:
void visit_struct(..., Error *errp)
{
if (!error_is_set(errp)) {
Error *err = NULL;
visit_start_struct(..., err);
if (!err) {
/* These functions set errors if none is set yet */
do_foo(err);
do_bar(err);
do_baz(err);
}
/* err is NULL here for success */
error_propagate(errp, err);
}
}
But you can find similar code for lists and unions. There's also code
like this:
/* err is NULL here for success */
error_propagate(errp, err);
err = NULL;
And then err can be assigned new errors, but the subsequent
error_propagate() will simply free the Error objects again.
Kevin
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
On Thu, Apr 10, 2014 at 1:48 AM, Markus Armbruster arm...@redhat.com wrote:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
That should be the callers problem. If you pass a NULL errp then the
intended semantic is to ignore errors.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
And this is irregular in that you are now mandating the Error ** and
thus removing the capability to ignore errors.
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
I think this code as-is is a good example of what we should do
elsewhere. The code base has bloated with the if (error) { bail; } on
every Error ** accepting API call. I proposed a while back a semantic
that Error ** Accepting APIs perform no action when the error is
already set to allow for long sequences of calls to run without the
constant checks. You then report the first error in a catchall at the
end of the run.
I think this particular code is probably good, provided your case of
NULL errp is enforced against by the caller.
Regards,
Peter
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
Peter Crosthwaite peter.crosthwa...@xilinx.com writes:
On Thu, Apr 10, 2014 at 1:48 AM, Markus Armbruster arm...@redhat.com wrote:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
That should be the callers problem. If you pass a NULL errp then the
intended semantic is to ignore errors.
The *caller* isn't interested in an error. But the callee's behavior
should *not* be affected by that at all other than not returning an
error.
In particular, the callee should never continue after an error just
because the caller isn't interested in detailed error information.
That's why if (error_is_set(errp)) bail and similar are always wrong
when errp is a parameter that may be null.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
And this is irregular in that you are now mandating the Error ** and
thus removing the capability to ignore errors.
It is irregular. The irregularity is necessary as long as the function
isn't prepared for a null errp.
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
I think this code as-is is a good example of what we should do
elsewhere. The code base has bloated with the if (error) { bail; } on
every Error ** accepting API call. I proposed a while back a semantic
that Error ** Accepting APIs perform no action when the error is
already set to allow for long sequences of calls to run without the
constant checks. You then report the first error in a catchall at the
end of the run.
I think this particular code is probably good, provided your case of
NULL errp is enforced against by the caller.
My point isn't that this technique is bad, only that it's different from
what we do everywhere else, and the two techniques do not combine well.
Here's how we handle errors everywhere else:
void frob([...], Error **errp)
{
Error *err = NULL;
foo(err)
if (err) {
goto out;
}
bar(err)
if (err) {
goto out;
}
[...]
out:
error_propagate(errp, err);
[...]
}
Both foo() and bar() are never entered with an error set. Consequently,
they don't check for that case.
If you screw up and call them with an error set, they'll die on the
first error of their own, because error_set() asserts the error is
clear.
You might be tempted to elide err, like this:
foo(errp)
if (error_is_set(errp)) {
goto out;
}
bar(errp)
if (error_is_set(errp)) {
goto out;
}
[...]
out:
[...]
But that's *wrong*, because it executes bar() after foo() failed when
errp is null. Ignoring errors from frob()
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
On Fri, Apr 11, 2014 at 11:41 PM, Markus Armbruster arm...@redhat.com wrote:
Peter Crosthwaite peter.crosthwa...@xilinx.com writes:
On Thu, Apr 10, 2014 at 1:48 AM, Markus Armbruster arm...@redhat.com wrote:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
That should be the callers problem. If you pass a NULL errp then the
intended semantic is to ignore errors.
The *caller* isn't interested in an error. But the callee's behavior
should *not* be affected by that at all other than not returning an
error.
In particular, the callee should never continue after an error just
because the caller isn't interested in detailed error information.
But the error is from a previous call not the current call. Callers
job to inform second call that first one failed (or in current status
quo - not call the second call at all). But its caller job to know the
dependancy otherwise calls are not self contained.
That's why if (error_is_set(errp)) bail and similar are always wrong
when errp is a parameter that may be null.
Agreed. Don't see the problem here though - it's bad caller code too.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
And this is irregular in that you are now mandating the Error ** and
thus removing the capability to ignore errors.
It is irregular. The irregularity is necessary as long as the function
isn't prepared for a null errp.
My understanding is all functions should be prepared for NULL errp.
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
I think this code as-is is a good example of what we should do
elsewhere. The code base has bloated with the if (error) { bail; } on
every Error ** accepting API call. I proposed a while back a semantic
that Error ** Accepting APIs perform no action when the error is
already set to allow for long sequences of calls to run without the
constant checks. You then report the first error in a catchall at the
end of the run.
I think this particular code is probably good, provided your case of
NULL errp is enforced against by the caller.
My point isn't that this technique is bad, only that it's different from
what we do everywhere else, and the two techniques do not combine well.
Here's how we handle errors everywhere else:
void frob([...], Error **errp)
{
Error *err = NULL;
foo(err)
if (err) {
goto out;
}
bar(err)
if (err) {
goto out;
}
[...]
out:
error_propagate(errp, err);
[...]
}
Both foo() and bar() are never entered with an error set. Consequently,
they don't check for that case.
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
Am 09.04.2014 um 17:48 hat Markus Armbruster geschrieben:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
I agree, use the same style as everywhere else.
The pattern in the generated visitor that I find more annoying, though,
is that it has a lot of code like:
if (!error_is_set(errp)) {
/* long block of code here */
}
And I believe there are even cases where this nests. There are also
error_propagate() calls that can (and do in the common case) propagate
NULL, this way selecting the first error, if any, but not stopping on
the first error. I always found it confusing to read that code.
Kevin
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
On 04/09/2014 09:48 AM, Markus Armbruster wrote:
I stumbled over this while trying to purge error_is_set() from the code.
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
Putting the tedium into the generated code is WHY we have generated
code; so that the rest of the code that is hand-written can be concise.
I like this latter idea of letting the visitors assume that errp is
clean on entry with the caller responsible for checking err after each step.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
On Wed, Apr 9, 2014 at 8:48 AM, Markus Armbruster arm...@redhat.com wrote:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
Opinions?
The original visiting code was loosely based on ASN1 marshaling code
from Samba which used the if error, bail out at the top style of
error handling.
As use of Error has evolved in QEMU, I agree that the paradigm of
bail out as soon as you see an error and fail fast is better so I'd
vote for changing the generated code to do that.
Regards,
Anthony Liguori
Re: [Qemu-devel] Error propagation in generated visitors and command marshallers
* Markus Armbruster (arm...@redhat.com) wrote:
I stumbled over this while trying to purge error_is_set() from the code.
Here's how we commonly use the Error API:
Error *err = NULL;
foo(arg, err)
if (err) {
goto out;
}
bar(arg, err)
if (err) {
goto out;
}
This ensures that err is null on entry, both for foo() and for bar().
Many functions rely on that, like this:
void foo(ArgType arg, Error **errp)
{
if (frobnicate(arg) 0) {
error_setg(errp, Can't frobnicate);
// This asserts errp != NULL
}
}
Here's how some of our visitor code uses the Error API (for real code,
check out generated qmp-marshal.c):
Error *err = NULL;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
Visitor *v = qmp_input_get_visitor(mi);
char *foo = NULL;
char *bar = NULL;
visit_type_str(v, foo, foo, err);
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Unlike above, this may pass a non-null errp to the second
visit_type_str(), namely when the first one fails.
Right, one of the problems is you just have long strings of visit_* calls
and adding a check to each one hides what you're actually doing in a sea
of checks. The downside is that if one of those visit's fails then you've
got no chance of figuring out which one it was.
In my BER world I've got some macros along the lines of:
#define LOCAL_ERR_REPORT(fallout) \
if (local_err) { \
fallout \
}
and at least then I can do things like:
visit_type_str(v, foo, foo, err);
LOCAL_ERR_REPORT( goto out; )
visit_type_str(v, bar, bar, err);
LOCAL_ERR_REPORT( goto out; )
which while not nice, means that you can actually follow the code, and
I can also add a printf to the macro to record the function/line so
that when one of them fails I can see which visit was the cause of the problem
(something that's currently very difficult).
The visitor functions guard against that, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
v-type_str(v, obj, name, errp);
}
}
As discussed before, error_is_set() is almost almost wrong, fragile or
unclean. What if errp is null? Then we fail to stop visiting after an
error.
The function could be improved like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
assert(errp);
if (!*errp) {
v-type_str(v, obj, name, errp);
}
}
But: is it a good idea to have both patterns in the code? Should we
perhaps use the common pattern for visiting, too? Like this:
visit_type_str(v, foo, foo, err);
if (err) {
goto out;
}
visit_type_str(v, bar, bar, err);
if (err) {
goto out;
}
Then we can assume *errp is clear on function entry, like this:
void visit_type_str(Visitor *v, char **obj, const char *name, Error
**errp)
{
v-type_str(v, obj, name, errp);
}
Should execute roughly the same number of conditional branches.
Tedious repetition of if (err) goto out in the caller, but that's what
we do elsewhere, and unlike elsewhere, these one's are generated.
The other problem is I had a tendency to typo some of the cases to
if (*err) and it's quite hard to spot and you wonder what's going on.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
