[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v5 02/11] qapi: allow QmpInputVisit
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v5 02/11] qapi: allow QmpInputVisitor to auto-cast types |
Date: |
Tue, 14 Jun 2016 14:25:53 +0100 |
User-agent: |
Mutt/1.6.1 (2016-04-27) |
On Thu, Jun 09, 2016 at 04:03:50PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <address@hidden> 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 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.
>
> We're still digging.
>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> > docs/qapi-code-gen.txt | 2 +-
> > include/qapi/qmp-input-visitor.h | 6 +-
> > qapi/opts-visitor.c | 1 +
> > qapi/qmp-input-visitor.c | 89 ++++++++++++++++++++++------
> > qmp.c | 2 +-
> > qom/qom-qobject.c | 2 +-
> > replay/replay-input.c | 2 +-
> > scripts/qapi-commands.py | 2 +-
> > tests/check-qnull.c | 2 +-
> > tests/test-qmp-commands.c | 2 +-
> > tests/test-qmp-input-strict.c | 2 +-
> > tests/test-qmp-input-visitor.c | 115
> > ++++++++++++++++++++++++++++++++++++-
> > tests/test-visitor-serialization.c | 2 +-
> > util/qemu-sockets.c | 2 +-
> > 14 files changed, 201 insertions(+), 30 deletions(-)
> > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> > index 4cf1cf8..00e4b1a 100644
> > --- a/qapi/opts-visitor.c
> > +++ b/qapi/opts-visitor.c
> > @@ -347,6 +347,7 @@ opts_type_bool(Visitor *v, const char *name, bool *obj,
> > Error **errp)
> > }
> >
> > if (opt->str) {
> > + /* Keep these values in sync with same code in qmp-input-visitor.c
> > */
>
> Also with parse_option_bool(). That's the canonical place, in fact.
....except parse_option_bool() doesn't allow "yes", "no", "y", "n" as valid
values - it only permits "on" and "off" :-( I guess I could make the
parse_option_bool() method non-static, make it accept these missing values,
and then call it from all places so we have guaranteed consistency.
>
> > if (strcmp(opt->str, "on") == 0 ||
> > strcmp(opt->str, "yes") == 0 ||
> > strcmp(opt->str, "y") == 0) {
> > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > index aea90a1..92023b1 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
> >
> > @@ -45,6 +46,7 @@ struct QmpInputVisitor
> >
> > /* True to reject parse in visit_end_struct() if unvisited keys
> > remain. */
> > bool strict;
> > + bool autocast;
> > };
> >
> > static QmpInputVisitor *to_qiv(Visitor *v)
> > @@ -254,15 +256,25 @@ static void qmp_input_type_int64(Visitor *v, const
> > char *name, int64_t *obj,
> > Error **errp)
> > {
> > 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) {
> > + if (qemu_strtoll(qstr->string, NULL, 10, obj) == 0) {
>
> In the commit message, you mentioned intended use: "with a QDict
> produced from QemuOpts, where everything is in string format." I figure
> you mean opts with empty opts->list->desc[]. For those, we accept any
> key and parse all values as strings.
>
> The idea with such QemuOpts is to *delay* parsing until we know how to
> parse. The delay should be transparent to the user. In particular,
> values should be parsed the same whether the parsing is delayed or not.
>
> The canonical QemuOpts value parser is qemu_opt_parse(). It parses
> integers with parse_option_number(). That function parses like
> stroull(qstr->string, ..., 0). Two differences:
>
> * stroll() vs. strtoull(): they differ in ERANGE behavior. This might
> be tolerable, but I haven't thought it through.
>
> * Base 0 vs 10: different syntax and semantics. Oops.
Sigh, I originally had my code using strtoull() directly as the
parse_option_number() method does, but had to change it to make
checkpatch.pl stfu thus creating incosistency. This is a great
example of why I hate the fact that we only validate our style
rules against new patches and not our existing codebase :-(
Anyway, it seems to be something where we should refactor code to
use the same parsing method from both places. eg by making the
parse_option_number method non-static and just calling it from
here.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
- Re: [Qemu-block] [Qemu-devel] [PATCH v5 01/11] qdict: implement a qdict_crumple method for un-flattening a dict, (continued)
[Qemu-block] [PATCH v5 04/11] util: add QAuthZ object as an authorization base class, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 03/11] qom: support arbitrary non-scalar properties with -object, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 02/11] qapi: allow QmpInputVisitor to auto-cast types, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 05/11] util: add QAuthZSimple object type for a simple access control list, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 06/11] acl: delete existing ACL implementation, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 07/11] qemu-nbd: add support for ACLs for TLS clients, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 09/11] migration: add support for a "tls-acl" migration parameter, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 08/11] nbd: allow an ACL to be set with nbd-server-start QMP command, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 10/11] chardev: add support for ACLs for TLS clients, Daniel P. Berrange, 2016/06/02
[Qemu-block] [PATCH v5 11/11] vnc: allow specifying a custom ACL object name, Daniel P. Berrange, 2016/06/02
Re: [Qemu-block] [PATCH v5 00/11] Provide a QOM-based authorization API, Daniel P. Berrange, 2016/06/08