On Fri, Nov 20, 2015 at 05:24:02PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabk...@redhat.com> writes:
>
> > On Fri, Nov 20, 2015 at 03:57:17PM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabk...@redhat.com> 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 <arm...@redhat.com>
> >> >
> >> > Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
> >> >
> >> > 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 <arm...@redhat.com>
> ---
> 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
