[Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev

[Markus Armbruster]

Gerd Hoffmann <address@hidden> writes:

>   Hi,
>
>> Gerd, what do you think about changing the contract for property parse
>> methods to return -EINVAL, -EBUSY and such?
>
> Hmm, wouldn't it make more sense to integrate with qerror somehow?

Depends.

If you want to move error reporting from qdev_prop_parse() and
set_property() into the parse methods, then simply use qemu_error() or
qemu_error_new() there:

+    qemu_error_new(QERR_SYNTAX, str);
     return -1;

This made a lot of sense if each parser threw largely different errors.
Which isn't the case: we have one so far ("failed to parse"), and we
just encountered a use for a second.

We have the same very few errors occuring in many parsers, and we report
them just the same, regardless of the parser used.  Michael's proposal
is optimized for that: it keeps the actual error reporting centralized
in one place, qdev_prop_parse(), and keeps the code occuring many times
to a bare minimum:

-    return -1;
+    return -EINVAL;

You could attempt to do the same with QError rather than errno.h codes.
Looks like this:

-    return -1;
+    return qemu_error_from_info2(__FILE__, __LINE__, __func__, QERR_SYNTAX, 
str);

where qemu_error_from_info2() is to qemu_error_from_info() what printf()
is to vprintf().  Naturally, you'd want sweep some of the ugliness under
a macro-rug, so it actually looks more like this:

-    return -1;
+    return qerror_new2(QERR_SYNTAX, str);

Except with a less unimaginative name, of course.

qdev_prop_parse() could then query the QError member error (a QDict) to
find out more about the error.  But there's no need for that.  All it
wants to know about the error is "was there one?".  We get:

-    if (prop->info->parse(dev, prop, value) != 0) {
+    qerr = prop->info->parse(dev, prop, value);
+    if (qerr) {
-        fprintf(stderr, "property \"%s.%s\": failed to parse \"%s\"\n",
-                dev->info->name, name, value);
+        qerror_emit(qerr);
         return -1;
     }

No improvement over letting the parsers call qemu_error_new().