From: Markus Armbruster <armbru@redhat.com>
References: <20181115140501.7872-1-david@redhat.com>
	<20181115140501.7872-7-david@redhat.com>
	<87muq972p0.fsf@dusky.pond.sub.org>
	<3d186921-e32f-60aa-cfc6-07345f6795c7@redhat.com>
Date: Mon, 19 Nov 2018 20:51:45 +0100
In-Reply-To: <3d186921-e32f-60aa-cfc6-07345f6795c7@redhat.com> (David
	Hildenbrand's message of "Mon, 19 Nov 2018 15:12:11 +0100")
Message-ID: <87o9akkfqm.fsf@dusky.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor
To: David Hildenbrand <david@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org, Michael Roth <mdroth@linux.vnet.ibm.com>, Igor Mammedov <imammedo@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>

Copying Igor and Eduardo for a hostmem.c bug.  Search for "core dumped".

David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>>> ---
>>>  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 :)

[...]