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

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

[Markus Armbruster]

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 <armbru@redhat.com>
---
 numa.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/numa.c b/numa.c
index fdfe294..a584e8e 100644
--- a/numa.c
+++ b/numa.c
@@ -568,6 +568,7 @@ static int query_memdev(Object *obj, void *opaque)
 
     return 0;
 error:
+    error_free(err);
     g_free(m->value);
     g_free(m);
 
@@ -576,15 +577,12 @@ error:
 
 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) {
+        /* FIXME propagate the error query_memdev() throws away */
+        error_setg(errp, "Unknown error");
         goto error;
     }
 
-- 
2.4.3

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

[Eduardo Habkost]

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 <armbru@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Do you know how to trigger a query_memdev() error today, or is
just theoretical?

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@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 <armbru@redhat.com>
>
> Reviewed-by: Eduardo Habkost <ehabkost@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?

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

[Eduardo Habkost]

On Fri, Nov 20, 2015 at 03:57:17PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@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 <armbru@redhat.com>
> >
> > Reviewed-by: Eduardo Habkost <ehabkost@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?)

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

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

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

[Eduardo Habkost]

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

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

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