[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Error propagation in generated visitors and command marshal
From: |
Markus Armbruster |
Subject: |
[Qemu-devel] Error propagation in generated visitors and command marshallers |
Date: |
Wed, 09 Apr 2014 17:48:44 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
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?
- [Qemu-devel] Error propagation in generated visitors and command marshallers,
Markus Armbruster <=
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Eric Blake, 2014/04/09
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Anthony Liguori, 2014/04/09
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Dr. David Alan Gilbert, 2014/04/09
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Kevin Wolf, 2014/04/10
- Re: [Qemu-devel] Error propagation in generated visitors and command marshallers, Peter Crosthwaite, 2014/04/11