Re: [Qemu-block] [Qemu-devel] [PATCH RFC v3 3/5] qapi: Permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/21/2017 03:01 PM, Markus Armbruster wrote:
>> From: "Daniel P. Berrange" <address@hidden>
>> 
>> Currently the QObjectInputVisitor assumes that all scalar values are
>> directly represented as the final types declared by the thing being
>> visited. i.e. it assumes an 'int' is using QInt, and a 'bool' is using
>> QBool, etc.  This is good when QObjectInputVisitor is fed a QObject
>> that came from a JSON document on the QMP monitor, as it will strictly
>> validate correctness.
>> 
>> To allow QObjectInputVisitor to be reused for visiting a QObject
>> originating from keyval_parse(), an alternative mode is needed where
>> all the scalars types are represented as QString and converted on the
>> fly to the final desired type.
>> 
>> Signed-off-by: Daniel P. Berrange <address@hidden>
>> Message-Id: <address@hidden>
>> 
>> Rebased, conflicts resolved, commit message updated to refer to
>> keyval_parse().
>> 
>> Control flow in qobject_input_type_number_autocast() simplified.
>> 
>> Additional tests in test-qemu-opts.c to verify QemuOpts compatibility.
>> To make the tests pass, use qemu_strtou64() instead of
>> parse_uint_full().
>> 
>> Use qemu_strtou64() and qemu_strtosz() instead of
>> parse_option_number() and parse_option_size() so we have to call
>> qobject_input_get_name() only when actually needed.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  include/qapi/qobject-input-visitor.h |  32 ++++-
>>  qapi/qobject-input-visitor.c         | 165 ++++++++++++++++++++++++
>>  tests/test-keyval.c                  | 241 
>> +++++++++++++++++++++++++++++++++++
>>  tests/test-qobject-input-visitor.c   | 194 +++++++++++++++++++++++++++-
>>  4 files changed, 624 insertions(+), 8 deletions(-)
>> 
>
>> @@ -71,6 +72,7 @@ static const char 
>> *qobject_input_get_name(QObjectInputVisitor *qiv,
>>              g_string_prepend(qiv->errname, name);
>>              g_string_prepend_c(qiv->errname, '.');
>>          } else {
>> +            /* TODO needs to be .%zu for autocast */
>>              snprintf(buf, sizeof(buf), "[%zu]", so->index);
>
> Ah, the TODO means you started thinking about list integration, and
> that's part of why this is still RFC.
>
>
>> +static void qobject_input_type_bool_autocast(Visitor *v, const char *name,
>> +                                             bool *obj, Error **errp)
>> +{
>> +    QObjectInputVisitor *qiv = to_qiv(v);
>> +    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
>> +    QString *qstr;
>> +    const char *str;
>> +
>> +    if (!qobj) {
>> +        return;
>> +    }
>> +    qstr = qobject_to_qstring(qobj);
>> +    if (!qstr) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
>> +                   qobject_input_get_name(qiv, name), "string");
>> +        return;
>> +    }
>> +
>> +    str = qstring_get_str(qstr);
>> +    if (!strcmp(str, "on")) {
>> +        *obj = true;
>> +    } else if (!strcmp(str, "off")) {
>> +        *obj = false;
>
> Why are we reimplementing only a subset of parse_option_bool() here?

We're reimplementing parse_option_bool() exactly:

    static void parse_option_bool(const char *name, const char *value, bool 
*ret,
                                  Error **errp)
    {
        if (!strcmp(value, "on")) {
            *ret = 1;
        } else if (!strcmp(value, "off")) {
            *ret = 0;
        } else {
            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                       name, "'on' or 'off'");
        }
    }

You're probably thinking of opts_type_bool(), which recognizes
additional spellings.  But options visitor compatibility is a headache
left for later.

> Overall, most of the changes from Dan's original look sane.

Thanks!