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

[Paolo Bonzini <pbonzini@redhat.com>]

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

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