Re: [Qemu-devel] [PATCH v2 3/3] string-input-visitor: Support alternate types

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4282 bytes --]

On 05/05/2017 03:11 PM, Eduardo Habkost wrote:
> When parsing alternates from a string, there are some limitations in
> what we can do, but it is a valid use case in some situations. We can
> support booleans, integer types, and enums.

Stale comment, given...

> 
> This will be used to support 'feature=force' in -cpu options, while
> keeping 'feature=on|off|true|false' represented as boolean values.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Updated string_input_visitor_new() documentation
>   to mention alternate support (Markus Armbruster)
> * Detect ambiguous alternates at runtime. Test case included.
> * Removed support for integers. We don't need it yet, and

...this.

>   it would require sorting out the parse_str() mess.
> * Change supported_qtypes to uint32_t (Eric Blake)
> * Update tests/qapi-schema/qapi-schema-test.out to match
>   qapi-schema-test.json updates
>   (Eric Blake)
> * Code indentation fix (Markus Armbruster)
> * Use &error_abort on test cases instead of g_assert(!err)
>   (Markus Armbruster)
> ---

>  
> +/*
> + * Check if alternate type string representation is ambiguous and
> + * can't be parsed by StringInputVisitor
> + */
> +static bool ambiguous_alternate(uint32_t qtypes, const char *const enum_table[])
> +{
> +    uint32_t non_str_qtypes = qtypes & ~(1U << QTYPE_QSTRING);
> +
> +    if ((qtypes & (1U << QTYPE_QSTRING)) && !enum_table && non_str_qtypes) {
> +        return true;
> +    }
> +
> +    if (qtypes & (1U << QTYPE_QBOOL)) {
> +        const char *const *e;
> +        /*
> +         * If the string representation of enum members can be parsed as
> +         * booleans, the alternate string representation is ambiguous.
> +         */
> +        for (e = enum_table; e && *e; e++) {
> +            if (try_parse_bool(*e, NULL) == 0) {
> +                return true;
> +            }
> +        }
> +    }
> +
> +    return false;
> +}

Seems okay for detecting ambiguity, but it is a runtime cost (one that
you will run every single time you parse, even though the answer will be
the same every single time you run it); I still think doing it at QAPI
compile time will be more efficient in the long run.  And as this is the
only use of enum_table added in 2/3, I'm still not sold on needing that
patch.

> +
> +static void start_alternate(Visitor *v, const char *name,
> +                            GenericAlternate **obj, size_t size,
> +                            uint32_t qtypes, const char *const enum_table[],
> +                            Error **errp)
> +{
> +    /*
> +     * Enum types are represented as QTYPE_QSTRING, so this is
> +     * the default. Actual parsing of the string as an enum is
> +     * done by visit_type_<EnumType>(), which is called just
> +     * after visit_start_alternate().
> +     */
> +    QType qtype = QTYPE_QSTRING;
> +    uint32_t unsupported_qtypes = qtypes & ~((1U << QTYPE_QSTRING) |
> +                                             (1U << QTYPE_QBOOL));
> +    StringInputVisitor *siv = to_siv(v);
> +
> +    if (ambiguous_alternate(qtypes, enum_table)) {
> +        error_setg(errp, "Can't parse ambiguous alternate type");
> +        return;
> +    }
> +
> +    if (unsupported_qtypes) {
> +        error_setg(errp, "Can't parse %s' alternate member",
> +                   QType_lookup[ctz32(unsupported_qtypes)]);
> +        return;
> +    }
> +
> +    if ((qtypes & (1U << QTYPE_QBOOL)) &&
> +        try_parse_bool(siv->string, NULL) == 0) {
> +        qtype = QTYPE_QBOOL;
> +    }
> +
> +    *obj = g_malloc0(size);
> +    (*obj)->type = qtype;

Slightly simpler for ignoring int for now, while still something we
could add in later.  I've been wanting to have an alternate for
'int'/'str' for InetAddress port, since we want to support named ports
but most often use just integers.  On the command line, port=1 is fine,
but in QMP, we currently have to spell it port="1".  That's a case where
we'd allow a pairing of any string with an integer, rather than just an
enum.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]