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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 14 Oct 2009 15:29:41 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 10/14/2009 12:34 AM, Markus Armbruster wrote:
> > Luiz Capitulino<lcapitulino@redhat.com>  writes:
> >
> >> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >> ---
> >>   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."

 I like both, but I see them as future improvements.

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

 We have qemu-monitor.hx, which is something like it.

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

 Not sure if it really helps.