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)
{
MemdevList **list = opaque;
MemdevList *m = NULL;
- Error *err = NULL;
if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) {
+ HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+
m = g_malloc0(sizeof(*m));
-
m->value = g_malloc0(sizeof(*m->value));
-
- m->value->size = object_property_get_int(obj, "size",
- &err);
- if (err) {
- goto error;
- }
-
- m->value->merge = object_property_get_bool(obj, "merge",
- &err);
- if (err) {
- goto error;
- }
-
- m->value->dump = object_property_get_bool(obj, "dump",
- &err);
- if (err) {
- goto error;
- }
-
- m->value->prealloc = object_property_get_bool(obj,
- "prealloc", &err);
- if (err) {
- goto error;
- }
-
- m->value->policy = object_property_get_enum(obj,
- "policy",
- "HostMemPolicy",
- &err);
- if (err) {
- goto error;
- }
-
+ m->value->size = backend->size;
+ m->value->merge = backend->merge;
+ m->value->dump = backend->dump;
+ m->value->prealloc = backend->prealloc;
+ m->value->policy = backend->policy;
object_property_get_uint16List(obj, "host-nodes",
- &m->value->host_nodes, &err);
- if (err) {
- goto error;
- }
-
+ &m->value->host_nodes, &error_abort);
m->next = *list;
*list = m;
}
return 0;
-error:
- g_free(m->value);
- g_free(m);
-
- return -1;
}
MemdevList *qmp_query_memdev(Error **errp)
{
- Object *obj;
+ Object *obj = object_get_objects_root();
MemdevList *list = NULL;
- obj = object_get_objects_root();
- if (obj == NULL) {
- return NULL;
- }
-
- if (object_child_foreach(obj, query_memdev, &list) != 0) {
- goto error;
- }
-
+ object_child_foreach(obj, query_memdev, &list);
return list;
-
-error:
- qapi_free_MemdevList(list);
- return NULL;
}
--
2.4.3
