[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: |
David Hildenbrand |
Subject: |
Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor |
Date: |
Mon, 19 Nov 2018 22:22:43 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
On 19.11.18 20:51, Markus Armbruster wrote:
> 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 :)
>
Haha, me too. Will add a short description to the patch message and
maybe resend tomorrow!
--
Thanks,
David / dhildenb
[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