Re: [Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists

[Luiz Capitulino]

On Mon, 22 Oct 2012 17:26:10 +0100
Peter Maydell <address@hidden> wrote:

> On 22 October 2012 16:35, Luiz Capitulino <address@hidden> wrote:
> > Peter Maydell <address@hidden> 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.