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

[Markus Armbruster]

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)
 {
     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