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 <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
  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)
---
 include/qapi/string-input-visitor.h     |  6 +-
 qapi/string-input-visitor.c             | 99 +++++++++++++++++++++++++++++----
 tests/test-string-input-visitor.c       | 76 +++++++++++++++++++++++++
 tests/qapi-schema/qapi-schema-test.json |  8 +++
 tests/qapi-schema/qapi-schema-test.out  |  9 +++
 5 files changed, 187 insertions(+), 11 deletions(-)

diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
index 33551340e3..e7f359f225 100644
--- a/include/qapi/string-input-visitor.h
+++ b/include/qapi/string-input-visitor.h
@@ -19,8 +19,12 @@ typedef struct StringInputVisitor StringInputVisitor;
 
 /*
  * The string input visitor does not implement support for visiting
- * QAPI structs, alternates, null, or arbitrary QTypes.  It also
+ * QAPI structs, null, or arbitrary QTypes.  It also
  * requires a non-null list argument to visit_start_list().
+ *
+ * Support for alternates is very limited: only bool and enum
+ * members are supported, and only when the enum members'
+ * representations can't be confused with a bool value.
  */
 Visitor *string_input_visitor_new(const char *str);
 
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index c089491c24..e339b88192 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -19,6 +19,7 @@
 #include "qemu/option.h"
 #include "qemu/queue.h"
 #include "qemu/range.h"
+#include "qemu/host-utils.h"
 
 
 struct StringInputVisitor
@@ -278,21 +279,34 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
     *obj = val;
 }
 
+static int try_parse_bool(const char *s, bool *result)
+{
+    if (!strcasecmp(s, "on") ||
+        !strcasecmp(s, "yes") ||
+        !strcasecmp(s, "true")) {
+        if (result) {
+            *result = true;
+        }
+        return 0;
+    }
+    if (!strcasecmp(s, "off") ||
+        !strcasecmp(s, "no") ||
+        !strcasecmp(s, "false")) {
+        if (result) {
+            *result = false;
+        }
+        return 0;
+    }
+
+    return -1;
+}
+
 static void parse_type_bool(Visitor *v, const char *name, bool *obj,
                             Error **errp)
 {
     StringInputVisitor *siv = to_siv(v);
 
-    if (!strcasecmp(siv->string, "on") ||
-        !strcasecmp(siv->string, "yes") ||
-        !strcasecmp(siv->string, "true")) {
-        *obj = true;
-        return;
-    }
-    if (!strcasecmp(siv->string, "off") ||
-        !strcasecmp(siv->string, "no") ||
-        !strcasecmp(siv->string, "false")) {
-        *obj = false;
+    if (try_parse_bool(siv->string, obj) == 0) {
         return;
     }
 
@@ -326,6 +340,70 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
     *obj = val;
 }
 
+/*
+ * 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;
+}
+
+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;
+}
+
 static void string_input_free(Visitor *v)
 {
     StringInputVisitor *siv = to_siv(v);
@@ -353,6 +431,7 @@ Visitor *string_input_visitor_new(const char *str)
     v->visitor.next_list = next_list;
     v->visitor.check_list = check_list;
     v->visitor.end_list = end_list;
+    v->visitor.start_alternate = start_alternate;
     v->visitor.free = string_input_free;
 
     v->string = str;
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 79313a7f7a..263fcc2b8c 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -290,6 +290,76 @@ static void test_visitor_in_enum(TestInputVisitorData *data,
     }
 }
 
+static void test_visitor_in_alt_bool_enum(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    Error *err = NULL;
+    Visitor *v;
+    AltBoolEnum *be = NULL;
+
+    v = visitor_input_test_init(data, "true");
+    visit_type_AltBoolEnum(v, NULL, &be, &error_abort);
+    g_assert_cmpint(be->type, ==, QTYPE_QBOOL);
+    g_assert(be->u.b);
+    qapi_free_AltBoolEnum(be);
+
+    v = visitor_input_test_init(data, "off");
+    visit_type_AltBoolEnum(v, NULL, &be, &error_abort);
+    g_assert_cmpint(be->type, ==, QTYPE_QBOOL);
+    g_assert(!be->u.b);
+    qapi_free_AltBoolEnum(be);
+
+    v = visitor_input_test_init(data, "value2");
+    visit_type_AltBoolEnum(v, NULL, &be, &error_abort);
+    g_assert_cmpint(be->type, ==, QTYPE_QSTRING);
+    g_assert_cmpint(be->u.e, ==, ENUM_ONE_VALUE2);
+    qapi_free_AltBoolEnum(be);
+
+    v = visitor_input_test_init(data, "value100");
+    visit_type_AltBoolEnum(v, NULL, &be, &err);
+    error_free_or_abort(&err);
+    qapi_free_AltBoolEnum(be);
+
+    v = visitor_input_test_init(data, "10");
+    visit_type_AltBoolEnum(v, NULL, &be, &err);
+    error_free_or_abort(&err);
+    qapi_free_AltBoolEnum(be);
+}
+
+static void test_visitor_in_alt_ambig_str(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    Error *err = NULL;
+    Visitor *v;
+    AltStrBool *sb = NULL;
+
+    /*
+     * AltStrBool has an ambiguous string representation, and
+     * can't be handled by string-input-visitor:
+     */
+    v = visitor_input_test_init(data, "s");
+    visit_type_AltStrBool(v, NULL, &sb, &err);
+    error_free_or_abort(&err);
+    qapi_free_AltStrBool(sb);
+}
+
+static void test_visitor_in_alt_ambig_enum(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    Error *err = NULL;
+    Visitor *v;
+    AltOnOffBool *ob = NULL;
+
+    /*
+     * AltOnOffBool has an ambiguous string representation, and
+     * can't be handled by string-input-visitor:
+     */
+    v = visitor_input_test_init(data, "on");
+    visit_type_AltOnOffBool(v, NULL, &ob, &err);
+    error_free_or_abort(&err);
+    qapi_free_AltOnOffBool(ob);
+}
+
 /* Try to crash the visitors */
 static void test_visitor_in_fuzz(TestInputVisitorData *data,
                                  const void *unused)
@@ -366,6 +436,12 @@ int main(int argc, char **argv)
                             &in_visitor_data, test_visitor_in_string);
     input_visitor_test_add("/string-visitor/input/enum",
                             &in_visitor_data, test_visitor_in_enum);
+    input_visitor_test_add("/string-visitor/input/alternate/bool_enum",
+                            &in_visitor_data, test_visitor_in_alt_bool_enum);
+    input_visitor_test_add("/string-visitor/input/alternate/ambig_enum",
+                            &in_visitor_data, test_visitor_in_alt_ambig_enum);
+    input_visitor_test_add("/string-visitor/input/alternate/ambig_str",
+                            &in_visitor_data, test_visitor_in_alt_ambig_str);
     input_visitor_test_add("/string-visitor/input/fuzz",
                             &in_visitor_data, test_visitor_in_fuzz);
 
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 842ea3c5e3..d602f3d40b 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -106,6 +106,14 @@
 { 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
 { 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
 
+# for testing string-input-visitor handling of alternates:
+{ 'alternate': 'AltBoolEnum', 'data': { 'b': 'bool', 'e': 'EnumOne' } }
+
+# this alternate type will be detected as ambiguous by string-input-visitor:
+{ 'enum': 'TestOnOffAuto',
+  'data': [ 'on', 'off', 'auto' ] }
+{ 'alternate': 'AltOnOffBool', 'data': { 'o': 'TestOnOffAuto', 'b': 'bool' } }
+
 # for testing native lists
 { 'union': 'UserDefNativeListUnion',
   'data': { 'integer': ['int'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 9d99c4eebb..17cd276295 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,3 +1,7 @@
+alternate AltBoolEnum
+    tag type
+    case b: bool
+    case e: EnumOne
 alternate AltIntNum
     tag type
     case i: int
@@ -10,6 +14,10 @@ alternate AltNumStr
     tag type
     case n: number
     case s: str
+alternate AltOnOffBool
+    tag type
+    case o: TestOnOffAuto
+    case b: bool
 alternate AltStrBool
     tag type
     case s: str
@@ -56,6 +64,7 @@ enum QEnumTwo ['value1', 'value2']
     prefix QENUM_TWO
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
     prefix QTYPE
+enum TestOnOffAuto ['on', 'off', 'auto']
 object TestStruct
     member integer: int optional=False
     member boolean: bool optional=False
-- 
2.11.0.259.g40922b1

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

On Fri, May 05, 2017 at 03:53:40PM -0500, Eric Blake wrote:
> 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.

Oops.

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

I'm OK with deleting this part. That's the main reason I moved it
to a separate function.

> 
> > +
> > +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.

Does that mean we already have an use case where we will have to
relax the restrictions on ambiguous enums? :)

I won't mind at all if we remove the runtime detection of
ambiguous enums. It will make the code much simpler.

-- 
Eduardo