[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plu
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak |
Date: |
Fri, 20 Nov 2015 20:44:21 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eduardo Habkost <address@hidden> writes:
> On Fri, Nov 20, 2015 at 05:24:02PM +0100, Markus Armbruster wrote:
>> 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)
> [...]
>> - 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.
- [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak, Markus Armbruster, 2015/11/20
- Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak, Eduardo Habkost, 2015/11/20
- Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak, Markus Armbruster, 2015/11/20
- Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak, Eduardo Habkost, 2015/11/20
- Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak, Markus Armbruster, 2015/11/20
- Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak, Eduardo Habkost, 2015/11/20
- Re: [Qemu-devel] [PATCH] numa: Clean up query-memdev error handling, plug leak,
Markus Armbruster <=