On Thu, 11 Feb 2010 16:27:00 +0100 Markus Armbruster <arm...@redhat.com> wrote:
> Anthony Liguori <anth...@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.
