| Message ID | 1334691381-7666-16-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 17/04/2012 21:36, Luiz Capitulino ha scritto: > + switch(qobject_type(obj)) { > + case QTYPE_QSTRING: > + qstring_append(arglist, > + qstring_get_str(qobject_to_qstring(obj))); > + break; Does this escape commas correctly? It seems much easier to use no_gen and qemu_opts_from_qdict... Then cmd_netdev_add can be void cmd_foo(QemuOpts *arglist, Error **errp); and later on we could even replace the QemuOpts with a visitor for full QAPI-ness... Paolo
On Tue, 17 Apr 2012 22:26:55 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 17/04/2012 21:36, Luiz Capitulino ha scritto: > > + switch(qobject_type(obj)) { > > + case QTYPE_QSTRING: > > + qstring_append(arglist, > > + qstring_get_str(qobject_to_qstring(obj))); > > + break; > > Does this escape commas correctly? No, but does it have to? Does QemuOpts accept an option with a coma in it? > It seems much easier to use no_gen and qemu_opts_from_qdict... Then > cmd_netdev_add can be netdev_add/del is expected to be a stable interface, so we can't use no_gen. > void cmd_foo(QemuOpts *arglist, Error **errp); Until now we're treating hmp.c like an external QMP C client, using QemuOpts this way will leak qemu internals to hmp.c... > > and later on we could even replace the QemuOpts with a visitor for full > QAPI-ness... > > Paolo >
Il 17/04/2012 22:42, Luiz Capitulino ha scritto: > On Tue, 17 Apr 2012 22:26:55 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Il 17/04/2012 21:36, Luiz Capitulino ha scritto: >>> + switch(qobject_type(obj)) { >>> + case QTYPE_QSTRING: >>> + qstring_append(arglist, >>> + qstring_get_str(qobject_to_qstring(obj))); >>> + break; >> >> Does this escape commas correctly? > > No, but does it have to? Does QemuOpts accept an option with a coma in it? Yes, ",," is parsed as ",". >> It seems much easier to use no_gen and qemu_opts_from_qdict... Then >> cmd_netdev_add can be > > netdev_add/del is expected to be a stable interface, so we can't use no_gen. You can have hmp_netdev_add and the no_gen qmp_netdev_add as front-ends for the QAPI cmd_netdev_add. I think it's fair when we have to take into account backwards-compatibility. The conversion gives correct error propagation, so even though QemuOpts still leaks it's a step in the right direction. >> void cmd_foo(QemuOpts *arglist, Error **errp); > > Until now we're treating hmp.c like an external QMP C client, using QemuOpts > this way will leak qemu internals to hmp.c... True, but on the other hand it sounds strange to have QAPI clients encoding options manually and escaping commas. A KeyValueList (list of string->string associations) could be an alternative, but I do think that ultimately we want to have a visitor and remove QemuOpts altogether from net.c. I can write a proof of concept in a couple of weeks. Again, we can proceed in steps. Paolo
On Wed, 18 Apr 2012 08:57:19 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 17/04/2012 22:42, Luiz Capitulino ha scritto: > > On Tue, 17 Apr 2012 22:26:55 +0200 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> Il 17/04/2012 21:36, Luiz Capitulino ha scritto: > >>> + switch(qobject_type(obj)) { > >>> + case QTYPE_QSTRING: > >>> + qstring_append(arglist, > >>> + qstring_get_str(qobject_to_qstring(obj))); > >>> + break; > >> > >> Does this escape commas correctly? > > > > No, but does it have to? Does QemuOpts accept an option with a coma in it? > > Yes, ",," is parsed as ",". The current code doesn't escape either... Either, it's because the user is expected to it him/herself or we don't have any option that accepts a coma. > >> It seems much easier to use no_gen and qemu_opts_from_qdict... Then > >> cmd_netdev_add can be > > > > netdev_add/del is expected to be a stable interface, so we can't use no_gen. > > You can have hmp_netdev_add and the no_gen qmp_netdev_add as front-ends > for the QAPI cmd_netdev_add. I think it's fair when we have to take > into account backwards-compatibility. The conversion gives correct > error propagation, so even though QemuOpts still leaks it's a step in > the right direction. I thought Anthony had plans to replace QemuOpts with something else, I think it was qcfg, but I might be wrong. Anthony?
Il 18/04/2012 14:51, Luiz Capitulino ha scritto: >>>> > >> It seems much easier to use no_gen and qemu_opts_from_qdict... Then >>>> > >> cmd_netdev_add can be >>> > > >>> > > netdev_add/del is expected to be a stable interface, so we can't use no_gen. >> > >> > You can have hmp_netdev_add and the no_gen qmp_netdev_add as front-ends >> > for the QAPI cmd_netdev_add. I think it's fair when we have to take >> > into account backwards-compatibility. The conversion gives correct >> > error propagation, so even though QemuOpts still leaks it's a step in >> > the right direction. > I thought Anthony had plans to replace QemuOpts with something else, > I think it was qcfg, but I might be wrong. Anthony? As far as I understood, QCFG is really the code name for a QemuOpts visitor. :) The idea is that instead of this: static int net_init_netdev(QemuOpts *opts, void *dummy) { return net_client_init(opts); } ... if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1) return -1; you automatically generate functions like these: static int netdev_cb(QemuOpts *opts, void *cb) { Error *err = NULL; NetdevOpts *o = NULL; int ret; QapiDeallocVisitor *md = qapi_dealloc_visitor_new(); QemuOptsVisitor *iv = qemu_opts_visitor_new(opts); Visitor *v; v = qemu_opts_get_visitor(iv); visit_type_NetdevOpts(v, (void **) &o, NULL, &err); qemu_opts_visitor_cleanup(iv); if (err) { qerror_report_err(err); ret = 1; } else { int (*p_cb)(NetdevOpts *) = cb; ret = p_cb(opts); } v = qapi_dealloc_get_visitor(ov); visit_type_NetdevOpts(v, (void **) &o, NULL, &errp); qapi_dealloc_visitor_cleanup(ov); } int netdev_foreach(int (*cb)(NetdevOpts *)) { return qemu_opts_foreach(qemu_find_opts("netdev"), netdev_cb, cb, 1); } and just do: netdev_foreach(net_client_init); There was more stuff in QCFG, including extensions to QemuOpts to represent an arbitrary QObject, but the above is pretty much it and is really what we need at the moment. Paolo
On 04/17/2012 03:26 PM, Paolo Bonzini wrote: > Il 17/04/2012 21:36, Luiz Capitulino ha scritto: >> + switch(qobject_type(obj)) { >> + case QTYPE_QSTRING: >> + qstring_append(arglist, >> + qstring_get_str(qobject_to_qstring(obj))); >> + break; > > Does this escape commas correctly? > > It seems much easier to use no_gen and qemu_opts_from_qdict... Then > cmd_netdev_add can be > > void cmd_foo(QemuOpts *arglist, Error **errp); > > and later on we could even replace the QemuOpts with a visitor for full > QAPI-ness... Yeah, I think that's a reasonable suggestion. Regards, Anthony Liguori > > Paolo >
On Wed, 18 Apr 2012 08:32:57 -0500 Anthony Liguori <aliguori@us.ibm.com> wrote: > On 04/17/2012 03:26 PM, Paolo Bonzini wrote: > > Il 17/04/2012 21:36, Luiz Capitulino ha scritto: > >> + switch(qobject_type(obj)) { > >> + case QTYPE_QSTRING: > >> + qstring_append(arglist, > >> + qstring_get_str(qobject_to_qstring(obj))); > >> + break; > > > > Does this escape commas correctly? > > > > It seems much easier to use no_gen and qemu_opts_from_qdict... Then > > cmd_netdev_add can be > > > > void cmd_foo(QemuOpts *arglist, Error **errp); > > > > and later on we could even replace the QemuOpts with a visitor for full > > QAPI-ness... > > Yeah, I think that's a reasonable suggestion. I hope you guys have reviewed the boring qemu-option changes too :)
Il 18/04/2012 16:12, Luiz Capitulino ha scritto: >> > Yeah, I think that's a reasonable suggestion. > I hope you guys have reviewed the boring qemu-option changes too :) Oh, those are perfectly fine. :) Paolo
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 0b4f0a0..1506263 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -122,6 +122,10 @@ bool has_%(argname)s = false; %(argtype)s %(argname)s; ''', argname=c_var(argname), argtype=c_type(argtype)) + if argtype == '**': + ret += mcgen(''' +QString *arglist = NULL; +''') pop_indent() return ret.rstrip() @@ -146,6 +150,8 @@ v = qmp_input_get_visitor(mi); obj=obj) for argname, argtype, optional, structured in parse_args(args): + if argtype == '**': + continue if optional: ret += mcgen(''' visit_start_optional(v, &has_%(c_name)s, "%(name)s", errp); @@ -257,6 +263,86 @@ def gen_marshal_input(name, args, ret_type, middle_mode): if (error_is_set(errp)) { goto out; } +''') + + # '**' implementation + regular_args = [] + var_list_name = "" + for argname, argtype, optional, structured in parse_args(args): + if argtype == '**': + var_list_name = argname + else: + regular_args.append(argname) + + if var_list_name: + ret += mcgen(''' + else { + const QDictEntry *entry; + + arglist = qstring_new(); + for (entry = qdict_first(args); entry; + entry = qdict_next(qdict, entry)) { + const QObject *obj = qdict_entry_value(entry); + char buf[32]; + int n; + +''') + for argname in regular_args: + ret += mcgen(''' + if (strcmp(qdict_entry_key(entry), "%(argname)s") == 0) { + continue; + } + +''', argname=argname) + + ret += mcgen(''' + if (qstring_len(arglist) > 0) { + qstring_append(arglist, ","); + } + + qstring_append(arglist, qdict_entry_key(entry)); + qstring_append(arglist, "="); + + switch(qobject_type(obj)) { + case QTYPE_QSTRING: + qstring_append(arglist, + qstring_get_str(qobject_to_qstring(obj))); + break; + case QTYPE_QINT: + n = snprintf(buf, sizeof(buf), %(arg1)s, + qint_get_int(qobject_to_qint(obj))); + assert(n < sizeof(buf)); + qstring_append(arglist, buf); + break; + case QTYPE_QFLOAT: + n = snprintf(buf, sizeof(buf), %(arg2)s, + qfloat_get_double(qobject_to_qfloat(obj))); + qstring_append(arglist, buf); + break; + case QTYPE_QBOOL: + pstrcpy(buf, sizeof(buf), + qbool_get_int(qobject_to_qbool(obj)) ? "on" : "off"); + qstring_append(arglist, buf); + break; + default: + QDECREF(arglist); + error_set(&local_err, QERR_INVALID_PARAMETER_TYPE, + qdict_entry_key(entry), + "string, int, float or bool"); + goto out; + } + } + + if (qstring_len(arglist) > 0) { + has_%(var_list_name)s = true; + %(var_list_name)s = qstring_get_str(arglist); + } + } + +''', var_list_name=c_var(var_list_name), arg1='"%" PRId64', arg2='"%.17g"', + arg3='"%d"') + + ret += mcgen(''' %(sync_call)s ''', sync_call=gen_sync_call(name, args, ret_type, indent=4)) @@ -270,6 +356,11 @@ out: visitor_input_block_cleanup=gen_visitor_input_block(args, None, dealloc=True)) + if var_list_name: + ret += mcgen(''' + QDECREF(arglist); +''') + if middle_mode: ret += mcgen(''' diff --git a/scripts/qapi.py b/scripts/qapi.py index e062336..9bd6e95 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -163,6 +163,8 @@ def c_type(name): return 'bool' elif name == 'number': return 'double' + elif name == '**': + return 'const char *' elif type(name) == list: return '%s *' % c_list_type(name[0]) elif is_enum(name):
The variable argument list allows QAPI functions to receive a string in the format "name1=value1,name2=value2 ... nameN=valueN". This is going to be used by QMP commands that accept options in QemuOpts format, like netdev_add. The variable argument list is represented by the QAPI type '**'. It's declared like this in the schema: { 'command': 'cmd-foo', 'data': { 'arglist': '**' } } and the code generator will generate the following signature for cmd-foo: void cmd_foo(const char *arglist, Error **errp); The argument list can be accessed in 'arglist'. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- scripts/qapi-commands.py | 91 ++++++++++++++++++++++++++++++++++++++++++++++ scripts/qapi.py | 2 + 2 files changed, 93 insertions(+)