Eduardo Habkost <ehabk...@redhat.com> writes: > 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.
Okay, will do that in v2.
