Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling

[Eduardo Habkost]

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 _abort,
>   and drop the useless error handling.
> 
> Messed up in commit 76b5d85 "qmp: add query-memdev".
> 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Eduardo Habkost 

Applied to the numa tree. Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling

[Markus Armbruster]

Markus Armbruster  writes:

> "Michael S. Tsirkin"  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 _abort,
>>>   and drop the useless error handling.
>>> 
>>> Messed up in commit 76b5d85 "qmp: add query-memdev".
>>> 
>>> Signed-off-by: Markus Armbruster 
>>
>>
>> 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.

Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling

[Michael S. Tsirkin]

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 _abort,
>   and drop the useless error handling.
> 
> Messed up in commit 76b5d85 "qmp: add query-memdev".
> 
> Signed-off-by: Markus Armbruster 


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",
> - );
> -if (err) {
> -goto error;
> -}
> -
> + _abort);
>  m->value->merge = object_property_get_bool(obj, "merge",
> -   );
> -if (err) {
> -goto error;
> -}
> -
> +   _abort);
>  m->value->dump = object_property_get_bool(obj, "dump",
> -  );
> -if (err) {
> -goto error;
> -}
> -
> +  _abort);
>  m->value->prealloc = object_property_get_bool(obj,
> -  "prealloc", );
> -if (err) {
> -goto error;
> -}
> -
> +  "prealloc",
> +  _abort);
>  m->value->policy = object_property_get_enum(obj,
>  "policy",
>  "HostMemPolicy",
> -);
> -if (err) {
> -goto error;
> -}
> -
> +_abort);
>  object_property_get_uint16List(obj, "host-nodes",
> -   >value->host_nodes, );
> -if (err) {
> -goto error;
> -}
> +   >value->host_nodes,
> +   _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, ) != 0) {
> -goto error;
> -}
> -
> +object_child_foreach(obj, query_memdev, );
>  return list;
> -
> -error:
> -qapi_free_MemdevList(list);
> -return NULL;
>  }
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling

[Markus Armbruster]

"Michael S. Tsirkin"  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 _abort,
>>   and drop the useless error handling.
>> 
>> Messed up in commit 76b5d85 "qmp: add query-memdev".
>> 
>> Signed-off-by: Markus Armbruster 
>
>
> 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.

[Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling

[Markus Armbruster]

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 _abort,
  and drop the useless error handling.

Messed up in commit 76b5d85 "qmp: add query-memdev".

Signed-off-by: Markus Armbruster 
---
 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",
- );
-if (err) {
-goto error;
-}
-
+ _abort);
 m->value->merge = object_property_get_bool(obj, "merge",
-   );
-if (err) {
-goto error;
-}
-
+   _abort);
 m->value->dump = object_property_get_bool(obj, "dump",
-  );
-if (err) {
-goto error;
-}
-
+  _abort);
 m->value->prealloc = object_property_get_bool(obj,
-  "prealloc", );
-if (err) {
-goto error;
-}
-
+  "prealloc",
+  _abort);
 m->value->policy = object_property_get_enum(obj,
 "policy",
 "HostMemPolicy",
-);
-if (err) {
-goto error;
-}
-
+_abort);
 object_property_get_uint16List(obj, "host-nodes",
-   >value->host_nodes, );
-if (err) {
-goto error;
-}
+   >value->host_nodes,
+   _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, ) != 0) {
-goto error;
-}
-
+object_child_foreach(obj, query_memdev, );
 return list;
-
-error:
-qapi_free_MemdevList(list);
-return NULL;
 }
-- 
2.4.3