All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling
@ 2015-11-23  8:35 Markus Armbruster
  2015-11-23  9:01 ` Michael S. Tsirkin
  2015-12-18 16:00 ` Eduardo Habkost
  0 siblings, 2 replies; 5+ messages in thread
From: Markus Armbruster @ 2015-11-23  8:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: ehabkost, mst

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 leaks err then.  But any failure
  there is actually a programming error.  Switch it to &error_abort,
  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 | 59 ++++++++++-------------------------------------------------
 1 file changed, 10 insertions(+), 49 deletions(-)

diff --git a/numa.c b/numa.c
index fdfe294..1710946 100644
--- a/numa.c
+++ b/numa.c
@@ -517,7 +517,6 @@ 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)) {
         m = g_malloc0(sizeof(*m));
@@ -525,72 +524,34 @@ static int query_memdev(Object *obj, void *opaque)
         m->value = g_malloc0(sizeof(*m->value));
 
         m->value->size = object_property_get_int(obj, "size",
-                                                 &err);
-        if (err) {
-            goto error;
-        }
-
+                                                 &error_abort);
         m->value->merge = object_property_get_bool(obj, "merge",
-                                                   &err);
-        if (err) {
-            goto error;
-        }
-
+                                                   &error_abort);
         m->value->dump = object_property_get_bool(obj, "dump",
-                                                  &err);
-        if (err) {
-            goto error;
-        }
-
+                                                  &error_abort);
         m->value->prealloc = object_property_get_bool(obj,
-                                                      "prealloc", &err);
-        if (err) {
-            goto error;
-        }
-
+                                                      "prealloc",
+                                                      &error_abort);
         m->value->policy = object_property_get_enum(obj,
                                                     "policy",
                                                     "HostMemPolicy",
-                                                    &err);
-        if (err) {
-            goto error;
-        }
-
+                                                    &error_abort);
         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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling
  2015-11-23  8:35 [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling Markus Armbruster
@ 2015-11-23  9:01 ` Michael S. Tsirkin
  2015-11-23 11:44   ` Markus Armbruster
  2015-12-18 16:00 ` Eduardo Habkost
  1 sibling, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2015-11-23  9:01 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, ehabkost

On Mon, Nov 23, 2015 at 09:35:31AM +0100, Markus Armbruster wrote:
> 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 leaks err then.  But any failure
>   there is actually a programming error.  Switch it to &error_abort,
>   and drop the useless error handling.
> 
> Messed up in commit 76b5d85 "qmp: add query-memdev".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


Post 2.5 right?

> ---
>  numa.c | 59 ++++++++++-------------------------------------------------
>  1 file changed, 10 insertions(+), 49 deletions(-)
> 
> diff --git a/numa.c b/numa.c
> index fdfe294..1710946 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -517,7 +517,6 @@ 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)) {
>          m = g_malloc0(sizeof(*m));
> @@ -525,72 +524,34 @@ static int query_memdev(Object *obj, void *opaque)
>          m->value = g_malloc0(sizeof(*m->value));
>  
>          m->value->size = object_property_get_int(obj, "size",
> -                                                 &err);
> -        if (err) {
> -            goto error;
> -        }
> -
> +                                                 &error_abort);
>          m->value->merge = object_property_get_bool(obj, "merge",
> -                                                   &err);
> -        if (err) {
> -            goto error;
> -        }
> -
> +                                                   &error_abort);
>          m->value->dump = object_property_get_bool(obj, "dump",
> -                                                  &err);
> -        if (err) {
> -            goto error;
> -        }
> -
> +                                                  &error_abort);
>          m->value->prealloc = object_property_get_bool(obj,
> -                                                      "prealloc", &err);
> -        if (err) {
> -            goto error;
> -        }
> -
> +                                                      "prealloc",
> +                                                      &error_abort);
>          m->value->policy = object_property_get_enum(obj,
>                                                      "policy",
>                                                      "HostMemPolicy",
> -                                                    &err);
> -        if (err) {
> -            goto error;
> -        }
> -
> +                                                    &error_abort);
>          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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling
  2015-11-23  9:01 ` Michael S. Tsirkin
@ 2015-11-23 11:44   ` Markus Armbruster
  2015-12-17  9:04     ` Markus Armbruster
  0 siblings, 1 reply; 5+ messages in thread
From: Markus Armbruster @ 2015-11-23 11:44 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, ehabkost

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Nov 23, 2015 at 09:35:31AM +0100, Markus Armbruster wrote:
>> 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 leaks err then.  But any failure
>>   there is actually a programming error.  Switch it to &error_abort,
>>   and drop the useless error handling.
>> 
>> Messed up in commit 76b5d85 "qmp: add query-memdev".
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>
> Post 2.5 right?

I don't mind.  I meant v1 for 2.5, but we've since convinced ourselves
that errors can't happen, and the memory leak is only latent.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling
  2015-11-23 11:44   ` Markus Armbruster
@ 2015-12-17  9:04     ` Markus Armbruster
  0 siblings, 0 replies; 5+ messages in thread
From: Markus Armbruster @ 2015-12-17  9:04 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, ehabkost

Markus Armbruster <armbru@redhat.com> writes:

> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> On Mon, Nov 23, 2015 at 09:35:31AM +0100, Markus Armbruster wrote:
>>> 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 leaks err then.  But any failure
>>>   there is actually a programming error.  Switch it to &error_abort,
>>>   and drop the useless error handling.
>>> 
>>> Messed up in commit 76b5d85 "qmp: add query-memdev".
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>>
>> Post 2.5 right?
>
> I don't mind.  I meant v1 for 2.5, but we've since convinced ourselves
> that errors can't happen, and the memory leak is only latent.

Friendly ping for 2.6.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling
  2015-11-23  8:35 [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling Markus Armbruster
  2015-11-23  9:01 ` Michael S. Tsirkin
@ 2015-12-18 16:00 ` Eduardo Habkost
  1 sibling, 0 replies; 5+ messages in thread
From: Eduardo Habkost @ 2015-12-18 16:00 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, mst

On Mon, Nov 23, 2015 at 09:35:31AM +0100, Markus Armbruster wrote:
> 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 leaks err then.  But any failure
>   there is actually a programming error.  Switch it to &error_abort,
>   and drop the useless error handling.
> 
> Messed up in commit 76b5d85 "qmp: add query-memdev".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

Applied to the numa tree. Thanks!

-- 
Eduardo

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-12-18 16:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-23  8:35 [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling Markus Armbruster
2015-11-23  9:01 ` Michael S. Tsirkin
2015-11-23 11:44   ` Markus Armbruster
2015-12-17  9:04     ` Markus Armbruster
2015-12-18 16:00 ` Eduardo Habkost

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.