Re: [Qemu-devel] [PATCH v8 3/7] qapi: add a QmpInputVisitor that does string conversion

[Markus Armbruster <armbru@redhat.com>]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> Currently the QmpInputVisitor assumes that all scalar
> values are directly represented as their final types.
> ie it assumes an 'int' is using QInt, and a 'bool' is
> using QBool.
>
> This adds an alternative constructor for QmpInputVisitor
> that will set it up such that it expects a QString for
> all scalar types instead.
>
> This makes it possible to use QmpInputVisitor with a
> QDict produced from QemuOpts, where everything is in
> string format.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qapi/qmp-input-visitor.h |  42 +++++++++--
>  qapi/qmp-input-visitor.c         | 106 ++++++++++++++++++++++++++++
>  tests/test-qmp-input-visitor.c   | 149 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 290 insertions(+), 7 deletions(-)
>
> diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h
> index f3ff5f3..33439b7 100644
> --- a/include/qapi/qmp-input-visitor.h
> +++ b/include/qapi/qmp-input-visitor.h
> @@ -19,12 +19,46 @@
>  
>  typedef struct QmpInputVisitor QmpInputVisitor;
>  
> -/*
> - * Return a new input visitor that converts QMP to QAPI.
> +/**
> + * qmp_input_visitor_new:
> + * @obj: the input object to visit
> + * @strict: whether to require that all input keys are consumed
> + *
> + * Create a new input visitor that converts QMP to QAPI.

Actually, it converts a QObject to a QAPI object.

> + *
> + * Any scalar values in the @obj input data structure should be in the
> + * required type already. ie if visiting a bool, the value should

i.e.

> + * already be a QBool instance.
>   *
> - * Set @strict to reject a parse that doesn't consume all keys of a
> - * dictionary; otherwise excess input is ignored.
> + * If @strict is set to true, then an error will be reported if any
> + * dict keys are not consumed during visitation.
> + *
> + * The returned input visitor should be released by calling
> + * qmp_input_visitor_cleanup when no longer required.
> + *
> + * Returns: a new input visitor
>   */
>  Visitor *qmp_input_visitor_new(QObject *obj, bool strict);
>  
> +/**
> + * qmp_string_input_visitor_new:
> + * @obj: the input object to visit
> + * @strict: whether to require that all input keys are consumed
> + *
> + * Create a new input visitor that converts QMP to QAPI.
> + *
> + * Any scalar values in the @obj input data structure should always be
> + * represented as strings. ie if visiting a boolean, the value should

i.e.

> + * be a QString whose contents represent a valid boolean.
> + *
> + * If @strict is set to true, then an error will be reported if any
> + * dict keys are not consumed during visitation.
> + *
> + * The returned input visitor should be released by calling
> + * qmp_input_visitor_cleanup when no longer required.
> + *
> + * Returns: a new input visitor
> + */
> +Visitor *qmp_string_input_visitor_new(QObject *obj, bool strict);

We have a QMP input visitor that isn't really about QMP, and a string
visitor.  You're adding a QMP string input visitor that's even less
about QMP (using it with QMP would be wrong, in fact), and that is
related to the string visitor only insofar as both parse strings.
Differently, of course; this is QEMU.

Can't think of a better name, and I'm running out of Friday.

Should we specify how strings are parsed?

Do you actually need both strict = true and strict = false here?

> +
>  #endif
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 21edb39..307155f 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -20,6 +20,7 @@
>  #include "qemu-common.h"
>  #include "qapi/qmp/types.h"
>  #include "qapi/qmp/qerror.h"
> +#include "qemu/cutils.h"
>  
>  #define QIV_STACK_SIZE 1024
>  
> @@ -268,6 +269,17 @@ static void qmp_input_type_int64(Visitor *v, const char *name, int64_t *obj,
>      *obj = qint_get_int(qint);
>  }
>  
> +static void qmp_input_type_int64_str(Visitor *v, const char *name, int64_t *obj,
> +                                     Error **errp)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +    QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));
> +    uint64_t ret;
> +
> +    parse_option_number(name, qstr ? qstr->string : NULL, &ret, errp);
> +    *obj = ret;
> +}
> +
>  static void qmp_input_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>                                    Error **errp)
>  {
> @@ -284,6 +296,15 @@ static void qmp_input_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>      *obj = qint_get_int(qint);
>  }
>  
> +static void qmp_input_type_uint64_str(Visitor *v, const char *name,
> +                                      uint64_t *obj, Error **errp)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +    QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));
> +
> +    parse_option_number(name, qstr ? qstr->string : NULL, obj, errp);
> +}
> +
>  static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj,
>                                  Error **errp)
>  {
> @@ -299,6 +320,15 @@ static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj,
>      *obj = qbool_get_bool(qbool);
>  }
>  
> +static void qmp_input_type_bool_str(Visitor *v, const char *name, bool *obj,
> +                                    Error **errp)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +    QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));
> +
> +    parse_option_bool(name, qstr ? qstr->string : NULL, obj, errp);
> +}
> +
>  static void qmp_input_type_str(Visitor *v, const char *name, char **obj,
>                                 Error **errp)
>  {
> @@ -339,6 +369,25 @@ static void qmp_input_type_number(Visitor *v, const char *name, double *obj,
>                 "number");
>  }
>  
> +static void qmp_input_type_number_str(Visitor *v, const char *name, double *obj,
> +                                      Error **errp)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +    QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));
> +    char *endp;
> +
> +    if (qstr && qstr->string) {
> +        errno = 0;
> +        *obj = strtod(qstr->string, &endp);
> +        if (errno == 0 && endp != qstr->string && *endp == '\0') {
> +            return;
> +        }
> +    }
> +
> +    error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +               "number");
> +}
> +
>  static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
>                                 Error **errp)
>  {
> @@ -360,6 +409,31 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
>      }
>  }
>  
> +static void qmp_input_type_size_str(Visitor *v, const char *name, uint64_t *obj,
> +                                    Error **errp)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +    QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));
> +    int64_t val;
> +    char *endptr;
> +
> +    if (qstr && qstr->string) {
> +        val = qemu_strtosz_suffix(qstr->string, &endptr,
> +                                  QEMU_STRTOSZ_DEFSUFFIX_B);
> +        if (val < 0 || *endptr) {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +                       "a size value representible as a non-negative int64");
> +            return;
> +        }
> +
> +        *obj = val;
> +        return;
> +    }
> +
> +    error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +               "size");
> +}
> +
>  static void qmp_input_optional(Visitor *v, const char *name, bool *present)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);

Separate callbacks result in cleaner code than what I reviewed last.
Cleaner semantics, too: either all the scalars have sane types, or all
are strings.

> @@ -411,3 +485,35 @@ Visitor *qmp_input_visitor_new(QObject *obj, bool strict)
>  
>      return &v->visitor;
>  }
> +
> +Visitor *qmp_string_input_visitor_new(QObject *obj, bool strict)
> +{
> +    QmpInputVisitor *v;
> +
> +    v = g_malloc0(sizeof(*v));
> +
> +    v->visitor.type = VISITOR_INPUT;
> +    v->visitor.start_struct = qmp_input_start_struct;
> +    v->visitor.check_struct = qmp_input_check_struct;
> +    v->visitor.end_struct = qmp_input_pop;
> +    v->visitor.start_list = qmp_input_start_list;
> +    v->visitor.next_list = qmp_input_next_list;
> +    v->visitor.end_list = qmp_input_pop;
> +    v->visitor.start_alternate = qmp_input_start_alternate;
> +    v->visitor.type_int64 = qmp_input_type_int64_str;
> +    v->visitor.type_uint64 = qmp_input_type_uint64_str;
> +    v->visitor.type_bool = qmp_input_type_bool_str;
> +    v->visitor.type_str = qmp_input_type_str;
> +    v->visitor.type_number = qmp_input_type_number_str;
> +    v->visitor.type_any = qmp_input_type_any;
> +    v->visitor.type_null = qmp_input_type_null;
> +    v->visitor.type_size = qmp_input_type_size_str;
> +    v->visitor.optional = qmp_input_optional;
> +    v->visitor.free = qmp_input_free;
> +    v->strict = strict;
> +
> +    v->root = obj;
> +    qobject_incref(obj);
> +
> +    return &v->visitor;
> +}
[...]