Re: [Qemu-devel] [PATCH 2/4] string-input-visitor: Support alternate types

[Eduardo Habkost]

On Tue, May 02, 2017 at 04:37:03PM -0500, Eric Blake wrote:
> On 05/02/2017 03:31 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.
> > 
> > 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 <address@hidden>
> > ---
> 
> >  
> > +/* Support for alternates on string-input-visitor is limited, because
> > + * the input string doesn't have any type information.
> > + *
> > + * Supported alternate member types:
> > + * 1) enums
> > + * 2) integer types
> > + * 3) booleans (but only if the there's no enum variant
> > + *    containing "on", "off", "true", or "false" as members)
> > + *
> > + * UNSUPPORTED alternate member types:
> > + * 1) strings
> > + * 2) complex types
> > + */
> > +static void start_alternate(Visitor *v, const char *name,
> > +                            GenericAlternate **obj, size_t size,
> > +                            unsigned long supported_qtypes, Error **errp)
> > +{
> > +    StringInputVisitor *siv = to_siv(v);
> > +    QType t = QTYPE_QSTRING;
> 
> Why do you document string as unsupported, and yet default to
> QTYPE_QSTRING?  I don't see how a string is fundamentally different from
> an enum (no alternate can have both at the same time, an alternate with
> either type will have QTYPE_QSTRING set in supported_qtypes).

Strings are indistinguishable from enums inside
start_alternate(), that's true. But I wanted to explicitly
document 'str' as unsupported by string-input-visitor because
some string values would necessarily overlap with the string
representation of other variants.

> 
> > +
> > +    if (supported_qtypes & BIT(QTYPE_QBOOL)) {
> > +        if (try_parse_bool(siv->string, NULL) == 0) {
> > +            t = QTYPE_QBOOL;
> > +        }
> > +    }
> > +
> > +    if (supported_qtypes & BIT(QTYPE_QINT)) {
> > +        if (parse_str(siv, name, NULL) == 0) {
> > +            t = QTYPE_QINT;
> > +        }
> > +    }
> > +
> > +    *obj = g_malloc0(size);
> > +    (*obj)->type = t;
> 
> Should you raise an error if you couldn't match the input with
> supported_qtypes, rather than just blindly returning QTYPE_QSTRING?

The generated visitors calling visit_start_alternate() already
generate a (more specific and more useful) error message when
they see unsupported types.

-- 
Eduardo