Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak

[Eduardo Habkost]

On Fri, Nov 20, 2015 at 03:57:17PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
> 
> > On Fri, Nov 20, 2015 at 02:00:40PM +0100, Markus Armbruster wrote:
> >> qmp_query_memdev() doesn't fail.  Instead, it returns an empty list.
> >> That's wrong.
> >> 
> >> Two error paths:
> >> 
> >> * When object_get_objects_root() returns null.  It never does, so
> >>   simply drop the useless error handling.
> >> 
> >> * When query_memdev() fails.  This can happen, and the error to return
> >>   is the one that query_memdev() currently leaks.  Passing the error
> >>   from query_memdev() to qmp_query_memdev() isn't so simple, because
> >>   object_child_foreach() is in the way.  Fixable, but I'd rather not
> >>   try it in hard freeze.  Plug the leak, make up an error, and add a
> >>   FIXME for the remaining work.
> >> 
> >> Screwed up in commit 76b5d85 "qmp: add query-memdev".
> >> 
> >> Signed-off-by: Markus Armbruster <address@hidden>
> >
> > Reviewed-by: Eduardo Habkost <address@hidden>
> >
> > Do you know how to trigger a query_memdev() error today, or is
> > just theoretical?
> 
> Theoretical; I tested by injecting an error with gdb.
> 
> query_memdev() fails exactly when some object_property_get_FOO() fails.
> If we decide such a failure would always be a programming error, we can
> pass &error_abort and simplify things.  Opinions?

The hostmem-backend property getters should never fail. Using
&error_abort on query_memdev() would make everything simpler.

(I would even use the HostMemoryBackend struct fields directly,
instead of QOM properties. Is there a good reason to use QOM to
fetch the data that's readily available in the C struct?)

-- 
Eduardo