From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NfbT6-0001IJ-FD for qemu-devel@nongnu.org; Thu, 11 Feb 2010 11:00:40 -0500 Received: from [199.232.76.173] (port=54154 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NfbT5-0001Ho-Ua for qemu-devel@nongnu.org; Thu, 11 Feb 2010 11:00:39 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NfbT4-0006hM-AK for qemu-devel@nongnu.org; Thu, 11 Feb 2010 11:00:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44254) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NfbT3-0006hE-Qm for qemu-devel@nongnu.org; Thu, 11 Feb 2010 11:00:38 -0500 Date: Thu, 11 Feb 2010 14:00:12 -0200 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling Message-ID: <20100211140012.58f8f0c7@doriath> In-Reply-To: References: <1265853007-27300-1-git-send-email-lcapitulino@redhat.com> <4B740E7A.9060105@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On Thu, 11 Feb 2010 16:27:00 +0100 Markus Armbruster wrote: > Anthony Liguori 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.