Re: [Qemu-devel] [PATCH v1 00/14]: Initial QObject conversion

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 02 Oct 2009 16:17:35 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On 10/02/09 15:47, Luiz Capitulino wrote:
> > On Fri, 02 Oct 2009 14:47:02 +0200
> > Gerd Hoffmann<kraxel@redhat.com>  wrote:
> >
> >>     Hi,
> >>
> >>>    Some people have suggested that we should have a better error handling
> >>> in the Monitor, in the meaning that error information should be correctly
> >>> propagated and handled in order to be used by the Monitor Protocol and
> >>> the existing user protocol.
> >>
> >> A bunch of code paths can be called from both monitor and non-monitor
> >> contexts (network configuration, device hotplug).  Right now there are
> >> the qemu_error*() functions to make sure the error messages appear on
> >> the correct place (monitor or stderr) without having to pass through a
> >> Monitor pointer all the way down.
> >>
> >> How do you plan to handle this in the new world of monitor error reporting?
> >
> >   Good question.
> >
> >   The first thing to bear in mind is that MonitorError is not just about
> > error reporting, but more importantly, it makes errors have a common
> > structure so that they can be emitted by the Monitor Protocol.
> 
> So maybe they shouldn't be named MonitorError in the first place?

 What do you suggest?

 Hm.. This could be QError, meaning that it's also a QObject,
so that we can put it in dicts, lists, etc.

> And make sure the design works for non-monitor contexts too?

 Yes, this is a good point.

> Having the user_error callback in struct mon_cmd_t doesn't make sense 
> from that point of view for example.

 If we make it qemu-wide, yes, I agree. There should be an error
table in QError with at least:

1. Error number/macro
2. Error description string
3. user_error() function

> Why user_error is needed in the first place btw?  To maintain 
> backward-compatible error message formating?

 Yes and to do pretty printing too.

 For example:

qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
           driver);

 Makes sense for humans, but for QMP it would probably look like:

{ "error": { "code": 1234,
             "desc": "device not found",
             "data": { "name": "foobar-device" } } }

 So, I would change the qemu_error() call to something like:

qemu_error_structed(QEMU_ERR_QDEV_NDEV, driver);

 This call would build the right QObjects in MonitorError (or QError),
and we would also need an user_error() function, like:

void qemu_err_qdev_nodev(const QError *error)
{
    const char *driver = qstring_get_str(error->data);
    qemu_error("Device \"%s\" not found.  Try -device '?' for a list.\n",
                driver);
}

 So, when QMP is disabled that function will be called to print the error
for humans. When QMP is enabled this is never called and the protocol
emission code will use QError to emit something like the { "error" }
dict above.

> >   One way to solve the problem could be to move the MonitorError pointer
> > to the Monitor struct.
> 
> I think that is a good idea nevertheless.

 Yes.

> > This way, when there's a ERR_SINK_MONITOR
> > qemu_error() would fill MonitorError instead of calling monitor_vprintf().
> >
> >   The bad news though, is that we would have to change all qemu_error()
> > calls to have "structured" errors instead of pretty printing, that is,
> > create macros, define its errors data and write functions to do
> > pretty printing....
> 
> Or add a qemu_error_structed() variant for smooth switchover and have 
> qemu_error() use a generic error code, so you don't have to switch over 
> all at once.  There are not *that* many qemu_error users though, so just 
> converting them all in one go might not be too hard.

 Ok.