[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor
From: |
David Hildenbrand |
Subject: |
Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor |
Date: |
Wed, 21 Nov 2018 11:53:57 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
On 20.11.18 21:58, Markus Armbruster wrote:
> I think the title should be something like
>
> qapi: Rewrite string-input-visitor's integer and list parsing
>
> because you don't actually rewrite all of it.
>
> Eric Blake <address@hidden> writes:
>
>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>> The input visitor has some problems right now, especially
>>> - unsigned type "Range" is used to process signed ranges, resulting in
>>> inconsistent behavior and ugly/magical code
>>> - uint64_t are parsed like int64_t, so big uint64_t values are not
>>> supported and error messages are misleading
>>> - lists/ranges of int64_t are accepted although no list is parsed and
>>> we should rather report an error
>>> - lists/ranges are preparsed using int64_t, making it hard to
>>> implement uint64_t values or uint64_t lists
>>> - types that don't support lists don't bail out
>>> - visiting beyond the end of a list is not handled properly
>>> - we don't actually parse lists, we parse *sets*: members are sorted,
>>> and duplicates eliminated
>>>
>>> So let's rewrite it by getting rid of usage of the type "Range" and
>>> properly supporting lists of int64_t and uint64_t (including ranges of
>>> both types), fixing the above mentioned issues.
>>>
>>> Lists of other types are not supported and will properly report an
>>> error. Virtual walks are now supported.
>>>
>>> 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.
>>>
>>> Please note that no users/callers have to be fixed up. Candiates using
>>
>> s/Candiates/Candidates/
>>
>>> visit_type_uint16List() and friends are:
>>> - backends/hostmem.c:host_memory_backend_set_host_nodes()
>>> -- Code can deal with dupilcates/unsorted lists
>>
>> s/dupilcates/duplicates/
Thanks, both fixed.
>
>>> @@ -330,9 +381,10 @@ static void parse_type_null(Visitor *v, const char
>>> *name, QNull **obj,
>>> {
>>> StringInputVisitor *siv = to_siv(v);
>>> + assert(siv->lm == LM_NONE);
>>> *obj = NULL;
>>> - if (!siv->string || siv->string[0]) {
>>> + if (siv->string[0]) {
>>
>> Why did this condition change?
>
> As far as I can tell, siv->string can't ever be null. Sticking the
> change into this patch is perhaps debatable. I'm okay with it.
Yes, we have an assertion when creating the visitor. Do you want me to
pull this into a separate patch?
(It made sense under the old patch subject ;) )
>
>> Reviewed-by: Eric Blake <address@hidden>
>
> With the commit message improved once more:
> Reviewed-by: Markus Armbruster <address@hidden>
>
Thanks!
--
Thanks,
David / dhildenb
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, (continued)
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Markus Armbruster, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Eric Blake, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, David Hildenbrand, 2018/11/21
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Markus Armbruster, 2018/11/21
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Eric Blake, 2018/11/21
[Qemu-devel] [PATCH v2 7/9] test-string-input-visitor: Use virtual walk, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 9/9] test-string-input-visitor: Add range overflow tests, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 8/9] test-string-input-visitor: Split off uint64 list tests, David Hildenbrand, 2018/11/20