qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak
Date: Fri, 20 Nov 2015 17:26:22 -0200
User-agent: Mutt/1.5.23 (2014-03-12)

On Fri, Nov 20, 2015 at 05:24:02PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
> 
> > 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?)
> 
> I can't see why not, sketch appended.  Note that I still go through the
> property for host_nodes, because I couldn't see how to do that simpler.
> If you like this patch, I'll post it as v2.
> 
> 
> 
> numa: Clean up query-memdev error handling
> 
> qmp_query_memdev() has two error paths:
> 
> * When object_get_objects_root() returns null.  It never does, so
>   simply drop the useless error handling.
> 
> * When query_memdev() fails.  It looks like it could, but that's just
>   because its code is pointlessly complicated.  Simplify it, and drop
>   the useless error handling.
> 
> Messed up in commit 76b5d85 "qmp: add query-memdev".
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  numa.c | 69 
> ++++++++++--------------------------------------------------------
>  1 file changed, 10 insertions(+), 59 deletions(-)
> 
> diff --git a/numa.c b/numa.c
> index fdfe294..4e9be9f 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -517,80 +517,31 @@ static int query_memdev(Object *obj, void *opaque)
[...]
> -        m->value->prealloc = object_property_get_bool(obj,
> -                                                      "prealloc", &err);
[...]
> +        m->value->prealloc = backend->prealloc;

This changes the QMP command result because the property getter
currently[1] returns backend->prealloc || backend->force_prealloc.

So, I stand corrected: there are at least two cases where using
the QOM property is useful at query_memdev(). Let's just use
&error_abort and keep all the existing object_property_get_*()
calls, then?

[1] I am not sure this is the right thing to do, but if we're
    going to change that, let's do it after 2.5.0.

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]