Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor

[David Hildenbrand <david@redhat.com>]

>>
>> 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)

Side node: I am not sure if there should be some range checks, but maybe
the bitmap is large enough .... hm ...

2. qapi-visit.c::visit_type_Memdev_members()

-> calls visit_type_uint16List()
-> I think this never used for input, only for output / freeing

3. qapi-visit.c::visit_type_NumaNodeOptions_members()

-> calls visit_type_uint16List()
-> I think this never used for input, only for output / freeing

4. qapi-visit.c::visit_type_RockerOfDpaGroup_members

-> calls visit_type_uint32List()
-> I think this never used for input, only for output / freeing

5. qapi-visit.c::visit_type_RxFilterInfo_members()

-> calls visit_type_intList()
-> I think this never used for input, only for output / freeing

6. numa.c::query_memdev()

-> calls object_property_get_uint16List()
--> String parsed via visit_type_uint16List() into list
-> qmp_query_memdev() uses this list
--> Not relevant if unique or sorted
-> 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.


Am I missing anything / is any of my statements wrong?

Thanks!

>>
>> diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
>> index 33551340e3..921f3875b9 100644
>> --- a/include/qapi/string-input-visitor.h
>> +++ b/include/qapi/string-input-visitor.h
>> @@ -19,8 +19,8 @@ typedef struct StringInputVisitor StringInputVisitor;
>>  
>>  /*
>>   * The string input visitor does not implement support for visiting
>> - * QAPI structs, alternates, null, or arbitrary QTypes.  It also
>> - * requires a non-null list argument to visit_start_list().
>> + * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
>> + * of integers (except type "size") are supported.
>>   */
>>  Visitor *string_input_visitor_new(const char *str);
>>  
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index b89c6c4e06..4113be01fb 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -4,10 +4,10 @@
>>   * Copyright Red Hat, Inc. 2012-2016
>>   *
>>   * Author: Paolo Bonzini <pbonzini@redhat.com>
>> + *         David Hildenbrand <david@redhat.com>
>>   *
>>   * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>>   * See the COPYING.LIB file in the top-level directory.
>> - *
>>   */
>>  
>>  #include "qemu/osdep.h"
>> @@ -18,21 +18,39 @@
>>  #include "qapi/qmp/qerror.h"
>>  #include "qapi/qmp/qnull.h"
>>  #include "qemu/option.h"
>> -#include "qemu/queue.h"
>> -#include "qemu/range.h"
>>  #include "qemu/cutils.h"
>>  

[...]
>>  static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
>>                               Error **errp)
>>  {
>>      StringInputVisitor *siv = to_siv(v);
>> -
>> -    if (parse_str(siv, name, errp) < 0) {
>> +    int64_t val;
>> +
>> +    switch (siv->lm) {
>> +    case LM_NONE:
>> +        /* just parse a simple int64, bail out if not completely consumed */
>> +        if (qemu_strtoi64(siv->string, NULL, 0, &val)) {
>> +                error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                           name ? name : "null", "int64");
>> +            return;
>> +        }
>> +        *obj = val;
>>          return;
>> +    case LM_UNPARSED:
>> +        if (try_parse_int64_list_entry(siv, obj)) {
>> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>> +                       "list of int64 values or ranges");
>> +            return;
>> +        }
>> +        assert(siv->lm == LM_INT64_RANGE);
>> +        /* FALLTHROUGH */
> 
> Please spell it /* fall through */, because:
> 
> $ git-grep -Ei '/\* *fall.?thr[a-z]* *\*/' | sed 's/.*\* *\(fall[^*]*\)\*.*/\1/i' | sort | uniq -c
>       4 FALL THROUGH 
>       8 FALLTHROUGH 
>      61 FALLTHRU 
>      36 Fall through 
>      20 Fallthrough 
>       4 Fallthru 
>     237 fall through 
>       1 fall-through 
>      16 fallthrough 
>      39 fallthru 

Done!

[...]
> 
>> +    case LM_INT64_RANGE:
>> +        /* return the next element in the range */
>> +        assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);
>> +        *obj = siv->rangeNext.i64++;
>> +
>> +        if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
>> +            /* end of range, check if there is more to parse */
>> +            if (siv->unparsed_string[0]) {
>> +                siv->lm = LM_UNPARSED;
>> +            } else {
>> +                siv->lm = LM_END;
>> +            }
> 
> I'd do
> 
>                siv->lm = siv->unparsed_string[0] ? LM_UNPARSED : LM_END;
> 
> Matter of taste, entirely up to you.

Yes, makes sense, thanks!

> 
>> +        }
>> +        return;
>> +    case LM_END:
>> +        error_setg(errp, "Fewer list elements expected");
>> +        return;
>> +    default:
>> +        abort();
>>      }
>> +}
>>  
>> -    if (!siv->ranges) {
>> -        goto error;
>> -    }
>> -
>> -    if (!siv->cur_range) {
>> -        Range *r;
>> +static int try_parse_uint64_list_entry(StringInputVisitor *siv, uint64_t *obj)
>> +{
>> +    const char *endptr;
>> +    uint64_t start, end;
>>  
>> -        siv->cur_range = g_list_first(siv->ranges);
>> -        if (!siv->cur_range) {
>> -            goto error;
>> +    /* parse a simple uint64 or range */
> 
> try_parse_int64_list_entry() doesn't have this comment.  I think either
> both or none should have it.  You decide.

Indeed, the comment got lost on the way. Will readd it.

[...]

-- 

Thanks,

David / dhildenb