* [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak
@ 2015-11-20 13:00 Markus Armbruster
2015-11-20 14:19 ` Eduardo Habkost
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2015-11-20 13:00 UTC (permalink / raw)
To: qemu-devel; +Cc: hutao, ehabkost, mst
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak
2015-11-20 13:00 [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak Markus Armbruster
@ 2015-11-20 14:19 ` Eduardo Habkost
2015-11-20 14:57 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2015-11-20 14:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: hutao, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak
2015-11-20 14:19 ` Eduardo Habkost
@ 2015-11-20 14:57 ` Markus Armbruster
2015-11-20 15:20 ` Eduardo Habkost
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2015-11-20 14:57 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: hutao, qemu-devel, mst
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak
2015-11-20 14:57 ` Markus Armbruster
@ 2015-11-20 15:20 ` Eduardo Habkost
2015-11-20 16:24 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2015-11-20 15:20 UTC (permalink / raw)
To: Markus Armbruster; +Cc: hutao, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak
2015-11-20 15:20 ` Eduardo Habkost
@ 2015-11-20 16:24 ` Markus Armbruster
2015-11-20 19:26 ` Eduardo Habkost
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2015-11-20 16:24 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: hutao, qemu-devel, mst
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak
2015-11-20 16:24 ` Markus Armbruster
@ 2015-11-20 19:26 ` Eduardo Habkost
2015-11-20 19:44 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2015-11-20 19:26 UTC (permalink / raw)
To: Markus Armbruster; +Cc: hutao, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak
2015-11-20 19:26 ` Eduardo Habkost
@ 2015-11-20 19:44 ` Markus Armbruster
0 siblings, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2015-11-20 19:44 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: hutao, qemu-devel, mst
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-20 19:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-20 13:00 [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak Markus Armbruster
2015-11-20 14:19 ` Eduardo Habkost
2015-11-20 14:57 ` Markus Armbruster
2015-11-20 15:20 ` Eduardo Habkost
2015-11-20 16:24 ` Markus Armbruster
2015-11-20 19:26 ` Eduardo Habkost
2015-11-20 19:44 ` Markus Armbruster
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.