Re: [Qemu-devel] [PATCH v3 02/10] qapi: allow QmpInputVisitor to auto-cast types

[Eric Blake <eblake@redhat.com>]

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

On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:
> 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 extends it so that QString is optionally permitted
> for any of the non-string scalar types. This behaviour
> is turned on by requesting the 'autocast' flag in the
> constructor.
> 
> This makes it possible to use QmpInputVisitor with a
> QDict produced from QemuOpts, where everything is in
> string format.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qapi/qmp-input-visitor.h |   3 +
>  qapi/qmp-input-visitor.c         |  96 +++++++++++++++++++++++++++-----
>  tests/test-qmp-input-visitor.c   | 115 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 196 insertions(+), 18 deletions(-)
> 
> diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h
> index 3ed499c..c25cb7c 100644
> --- a/include/qapi/qmp-input-visitor.h
> +++ b/include/qapi/qmp-input-visitor.h
> @@ -21,6 +21,9 @@ typedef struct QmpInputVisitor QmpInputVisitor;
>  
>  QmpInputVisitor *qmp_input_visitor_new(QObject *obj);
>  QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj);
> +QmpInputVisitor *qmp_input_visitor_new_full(QObject *obj,
> +                                            bool strict,
> +                                            bool autocast);

We have so few uses of qmp_input_visitor_new* that it might be worth
just having a single prototype, and maybe using an 'int flags' instead
of a string of bool.  But not a show-stopper for this patch (rather, an
idea for a future patch).


> -    *obj = qint_get_int(qint);
> +    qstr = qobject_to_qstring(qobj);
> +    if (qstr && qstr->string && qiv->autocast) {
> +        errno = 0;

Dead setting of errno, since...

> +        if (qemu_strtoll(qstr->string, NULL, 10, obj) == 0) {

qemu_strtoll() handles it on your behalf, and you aren't using
error_setg_errno().

> @@ -233,30 +245,61 @@ static void qmp_input_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>  {
>      /* FIXME: qobject_to_qint mishandles values over INT64_MAX */
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
> +    QObject *qobj = qmp_input_get_object(qiv, name, true);
> +    QInt *qint;
> +    QString *qstr;
>  
> -    if (!qint) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> -                   "integer");
> +    qint = qobject_to_qint(qobj);
> +    if (qint) {
> +        *obj = qint_get_int(qint);
>          return;
>      }
>  
> -    *obj = qint_get_int(qint);
> +    qstr = qobject_to_qstring(qobj);
> +    if (qstr && qstr->string && qiv->autocast) {
> +        errno = 0;
> +        if (qemu_strtoull(qstr->string, NULL, 10, obj) == 0) {

And again.

Hmm.  Do we need to worry about partial asymmetry?  That is,
qint_get_int() returns a signed number, but qemu_strtoull() parses
unsigned; if the original conversion from JSON to qint uses a different
parser, then we could have funny results where we get different results
for things like:
 "key1":9223372036854775807, "key2":"9223372036854775807",
even though the same string of digits is being parsed, based on whether
the different parsers handle numbers larger than INT64_MAX differently.

[Ultimately, I'd like QInt to be enhanced to track whether the input was
signed or unsigned, and automatically make the output match the input
when converting back to string; that is, track 65 bits of information
instead of 64; but that's no sooner than 2.7 material]


>  static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj,
>                                  Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, true));
> +    QObject *qobj = qmp_input_get_object(qiv, name, true);
> +    QBool *qbool;
> +    QString *qstr;
>  
> -    if (!qbool) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> -                   "boolean");
> +    qbool = qobject_to_qbool(qobj);
> +    if (qbool) {
> +        *obj = qbool_get_bool(qbool);
>          return;
>      }
>  
> -    *obj = qbool_get_bool(qbool);
> +
> +    qstr = qobject_to_qstring(qobj);
> +    if (qstr && qstr->string && qiv->autocast) {
> +        if (!strcasecmp(qstr->string, "on") ||
> +            !strcasecmp(qstr->string, "yes") ||
> +            !strcasecmp(qstr->string, "true")) {
> +            *obj = true;
> +            return;
> +        }

Do we also want to allow "0"/"1" for true/false?

Overall, I'm a big fan of this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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