[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor |
Date: |
Mon, 19 Nov 2018 20:51:45 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Copying Igor and Eduardo for a hostmem.c bug. Search for "core dumped".
David Hildenbrand <address@hidden> writes:
>>>
>>> Tests have to be fixed up:
>>> - Two BUGs were hardcoded that are fixed now
>>> - The string-input-visitor now actually returns a parsed list and not
>>> an ordered set.
>>
>> I'd expect this to necessitate an update of callers that expect a set, but...
>>
>>> Signed-off-by: David Hildenbrand <address@hidden>
>>> ---
>>> include/qapi/string-input-visitor.h | 4 +-
>>> qapi/string-input-visitor.c | 410 ++++++++++++++++------------
>>> tests/test-string-input-visitor.c | 18 +-
>>> 3 files changed, 239 insertions(+), 193 deletions(-)
>>
>> ... there's none.
>>
>> Let me know if you need help finding them. I think we tracked them down
>> during the discussion that led to this series.
>>
>
> Indeed, I missed to document that. So here is the outcome:
>
> 1. backends/hostmem.c:host_memory_backend_set_host_nodes()
>
> -> calls visit_type_uint16List(via bitmap)
> -> the code can deal with duplicates/unsorted lists (bitmap_set)
Yes.
> Side node: I am not sure if there should be some range checks, but maybe
> the bitmap is large enough .... hm ...
Fishy. MAX_NODES is 128. Tinker, tinker, ...
$ upstream-qemu -nodefaults -object
memory-backend-file,id=mem0,mem-path=x,size=4096,host-nodes=12345
Segmentation fault (core dumped)
Igor, Eduardo, this is yours.
There's another use of visit_type_uint16List() is this file, but it's in
property getter host_memory_backend_get_host_nodes(), and property
getters aren't used with the string input visitor.
> 2. qapi-visit.c::visit_type_Memdev_members()
>
> -> calls visit_type_uint16List()
> -> I think this never used for input, only for output / freeing
Yes, it's used by query-memdev with the QObject output visitor to build
the value of @host-nodes.
> 3. qapi-visit.c::visit_type_NumaNodeOptions_members()
>
> -> calls visit_type_uint16List()
> -> I think this never used for input, only for output / freeing
It's used for input, but with the opts visitor, see parse_numa().
> 4. qapi-visit.c::visit_type_RockerOfDpaGroup_members
>
> -> calls visit_type_uint32List()
> -> I think this never used for input, only for output / freeing
Yes, it's used by query-rocker-of-dpa-groups with the QObject output
visitor to build the value of @group-ids.
> 5. qapi-visit.c::visit_type_RxFilterInfo_members()
>
> -> calls visit_type_intList()
> -> I think this never used for input, only for output / freeing
Yes, it's used by query-rx-filter with the QObject output visitor to
build the value of @vlan-table.
> 6. numa.c::query_memdev()
>
> -> calls object_property_get_uint16List()
> --> String parsed via visit_type_uint16List() into list
QOM, hard to understand.
The value of struct HostMemoryBackend member @host-nodes (a bitmap) is
first converted to a list (sorted, no duplicates) with
host_memory_backend_get_host_nodes() via object_property_get(), then
converted to a string with the string output visitor. The resulting
string is then converted back to a list with the string input visitor.
Despite the shenanigans going on in the string output visitor, I'd
expect the resulting list to also be sorted and without duplicates.
> -> qmp_query_memdev() uses this list
> --> Not relevant if unique or sorted
Depends on the contract of QMP command query-memdev. Here's the
relevant part.
# @host-nodes: host nodes for its memory policy
Useless.
"Sorted, no duplicates" might have become de facto ABI. Not sure.
However, I believe your patch won't affect it, as per the argument I
just made.
> -> hmp_info_memdev() uses this list
> --> List converted again to a string using string output visitor
>
> -> I don't think unique/sorted is relevant here.
HMP is not a stable interface.
> Am I missing anything / is any of my statements wrong?
Searching the QAPI schema for lists of integers coughs up block latency
histogram stuff, but that's unrelated, as far as I can tell.
Looks like we're good. I didn't expect that :)
[...]
[Qemu-devel] [PATCH v1 8/9] test-string-input-visitor: split off uint64 list tests, David Hildenbrand, 2018/11/15
[Qemu-devel] [PATCH v1 9/9] test-string-input-visitor: add range overflow tests, David Hildenbrand, 2018/11/15