[Qemu-devel] Re: [PATCH 7/9] qdev: Use QError for not found error

[Paolo Bonzini]

On 10/14/2009 12:34 AM, Markus Armbruster wrote:
> Luiz Capitulino<address@hidden>  writes:
>
> > Signed-off-by: Luiz Capitulino<address@hidden>
> > ---
> >   hw/qdev.c |    4 ++--
> >   1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 906e897..3ce48f7 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -28,6 +28,7 @@
> >   #include "net.h"
> >   #include "qdev.h"
> >   #include "sysemu.h"
> > +#include "qerror.h"
> >   #include "monitor.h"
> >
> >   static int qdev_hotplug = 0;
> > @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >       /* find driver */
> >       info = qdev_find_info(NULL, driver);
> >       if (!info) {
> > -        qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
> > -                   driver);
> > +        qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver);
> >           return NULL;
> >       }
> >       if (info->no_user) {
>
> And here we pay the price for structured error objects: reporting an
> error becomes more difficult.  The harder you make reporting errors, the
> fewer errors get caught and reported.  Also, the result is harder to
> read.

If you count also the new code in 6/9, reporting an error definitely 
becomes harder.  Finding a middle ground would be great, and I think 
that the right solution is to put as many things as possible in a single 
place.  For example:

1) there is a lot of code that is duplicated in every occurrence of an 
error.  One possibility to avoid this, would be to use a more 
printf-like syntax for qemu_object_from_va, for example one of these:

     .... "{ name: %s }", driver);
     .... "{ 'name': %s }", driver);

Then you can move the first argument to the error table:

+        .code   = QERR_QDEV_NFOUND,
+        .desc   = "device not found",
+        .keys   = "{ name: %s }"

so that the qemu_error_structed call looks like

    qemu_error_structed (QERR_QDEV_NFOUND, driver);

2) do we need full flexibility of a function for printing errors?  What 
about adding a print-from-qobject function, so that you could replace 
qerror_table[].user_print with a string like

         .formatted_print = "Device \"%{name}\" not found.  Try -device"
                            " '?' for a list."

3) I don't think this technique is used in qemu, but what about putting 
all errors in a header file like

QEMU_ERROR(QERR_DEV_NFOUND,
           "device not found",
           whatever);

Then you can use the header file to generate both the enum and the error 
table:

typedef enum QErrorCode {
#define QEMU_ERROR(code_, ...) code_,
#include "qemu.def"
#undef QEMU_ERROR
} QErrorCode;

...

static QErrorTable qerror_table[] = {
#define QEMU_ERROR(code_, desc_, data_) \
    { .code = (code_), .desc = (desc_), .data = (data_),
#include "qemu-error.def"
#undef QEMU_ERROR
};

This has the disadvantage of not allowing usage of designated initializers.

Paolo