From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQLDG-00050f-8K for qemu-devel@nongnu.org; Mon, 22 Oct 2012 12:50:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQLDD-0003QF-82 for qemu-devel@nongnu.org; Mon, 22 Oct 2012 12:50:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQLDC-0003Pp-PI for qemu-devel@nongnu.org; Mon, 22 Oct 2012 12:50:46 -0400 Date: Mon, 22 Oct 2012 14:51:39 -0200 From: Luiz Capitulino Message-ID: <20121022145139.2fe39342@doriath.home> In-Reply-To: References: <1350667146-26273-1-git-send-email-peter.maydell@linaro.org> <20121022133520.5515bd9f@doriath.home> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Paolo Bonzini , Anthony Liguori , qemu-devel@nongnu.org, patches@linaro.org On Mon, 22 Oct 2012 17:26:10 +0100 Peter Maydell wrote: > On 22 October 2012 16:35, Luiz Capitulino wrote: > > Peter Maydell wrote: > > > >> The aim of this patch series is to make QEMU exit with a helpful > >> error message for bugs where multiple properties of the same name > >> are accidentally added to a QOM object. > > > > Does this happen only at build-time or can it happen at command-line > > too? What about QMP/HMP? > > Anything that cares about not abort()ing needs to pass a valid > Error** in. That's not clear for a function called error_propagate(). Besides, we've been using error_propagate() without assuming it could abort() for some time now, I really don't feel this is safe to do. > >> In order to achieve this > >> for static properties whilst still allowing the hotplug case > >> to gracefully fail without killing QEMU, we add the concept > >> of a 'critical' error. A critical error is one which must be > >> handled somehow -- if we encounter a NULL Error** either when > >> the error is raised or later when it is propagated, we will > >> abort() rather than throwing the error away. > > > > This gives me the impression that we're fixing it in the wrong layer. > > Besides, all code calling error_propagate() today can now abort > > (at least in theory), but that's something we really don't want to happen > > in QMP. > > That's why QMP gets to pass in an Error ** and handle the result. > > I'm open to better ways to handle this. Perhaps all Errors should > be critical? :-) What about moving the decision to abort or not abort to the call sites instead? Ie. you add the is_critical bool plus error_is_critical(), but drop the automatic abort? Or add error_abort_if_critical(). > Mostly what this patch is trying to do is deal with the fact that > huge amounts of code using the Error interface just throws away > the error. It's up to the caller of a function taking an Error ** object to decide whether or not to ignore an error. If certain callers choose to do so, than I can only assume that that was the correct behavior chosen by whom wrote the code. If this turns out not be right, than randomly aborting is not the right thing to do either. My suggestion is to either fix the code paths not to ignore errors or move the abort() to the call sites where aborting is the right thing to do.