Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 11 Feb 2010 16:27:00 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > On 02/10/2010 07:49 PM, Luiz Capitulino wrote:
> >>   Hi there,
> >>
> >>   When I started converting handlers to the QObject style, I thought that
> >> returning an error code wouldn't be needed. That is, we have an error object
> >> already, so if the handler returns the error object it has failed, otherwise
> >> it has succeeded.
> >>
> >>   This was also very convenient, because handlers have never returned an error
> >> code, and thus it would be easier to add a call to qemu_error_new() in the
> >> right place instead of returning error codes.
> >>
> >>   Turns out we need both. Actually, I should not have abused the error object
> >> this way because (as Markus says) this is too fragile and we can end up
> >> reporting bogus errors to clients (among other problems).
> >>
> >>   The good news is that it's easy to fix.
> >>
> >>   All we have to do is to change cmd_new() (the handler callback) to return an
> >> error code and convert existing QObject handlers to it. This series does that
> >> and most of the patches are really straightforward conversions.
> >>
> >>   Additionally, Markus has designed an excellent debug mechanism for QMP, which
> >> is implemented by the last patches in this series, and will allow us to catch
> >> important QObject conversion and error handling bugs in handlers.
> >>    
> >
> > Instead of returning -1, would it make more sense to return an error
> > object?  If fact, why not drop ret_data as a passed in parameter, and
> > just always return either the result or an error object.
> 
> Tempting, isn't it?
> 
> The practical trouble with this idea is that you have to adjust a lot of
> code to return error objects all the way up into the handler.  With the
> current design, you emit error objects "sideways", into the monitor
> state.  This lets us keep the current mechanisms to report success /
> failure (return >= 0 / -1; non-NULL / NULL, non-zero / zero, ...).

 Yes, also note that some functions are used by other subsystems. If they
return -1/0 we'd have to adjust other call chains as well.