| Message ID | 1333042003-15490-13-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/29/2012 12:26 PM, Luiz Capitulino wrote: > This allows for QAPI functions to receive a variable-length argument > list. This is going to be used by device_add and netdev_add commands. > > In the schema, the argument list is represented by type name '**', > like this example: > > { 'command': 'foo', 'data': { 'arg-list': '**' } } > > Each argument is represented by the KeyValues type and the C > implementation should expect a KeyValuesList, like: > > void qmp_foo(KeyValuesList *values_list, Error **errp); > > XXX: This implementation is simple but very hacky. We just iterate > through all arguments and build the KeyValuesList list to be > passed to the QAPI function. > > Maybe we could have a kwargs type, that does exactly this but > through a visitor instead? > > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> What about just treating '**' as "marshal remaining arguments to a string" and then pass that string to device_add? qmp_device_add can then parse that string with QemuOpts. It's a bit ugly, but that's how things worked. When we introduce qom_add, this problem goes away because you would make multiple calls to qom_set to set all of the properties. Regards, Anthony Liguori > --- > qapi-schema.json | 15 +++++++++++++++ > scripts/qapi-commands.py | 31 ++++++++++++++++++++++++++++--- > scripts/qapi.py | 2 ++ > 3 files changed, 45 insertions(+), 3 deletions(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 0d11d6e..25bd487 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1701,3 +1701,18 @@ > # Since: 1.1 > ## > { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} } > + > +## > +# @KeyValues: > +# > +# A generic representation of a key value pair. > +# > +# @key: the name of the item > +# > +# @value: the string representation of the item value. This typically follows > +# QEMU's command line parsing format. See the man pages for more > +# information. > +# > +# Since: 0.14.0 > +## > +{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} } > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 30a24d2..75a6e81 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi); > obj=obj) > > for argname, argtype, optional, structured in parse_args(args): > - if optional: > + if optional and not '**': > ret += mcgen(''' > visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp); > if (has_%(c_name)s) { > ''', > c_name=c_var(argname), name=argname) > push_indent() > - ret += mcgen(''' > + if argtype == '**': > + if dealloc: > + ret += mcgen(''' > +qapi_free_KeyValuesList(%(obj)s); > +''', > + obj=c_var(argname)) > + else: > + ret += mcgen(''' > +{ > + const QDictEntry *entry; > + v = v; /* fix me baby */ > + > + for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) { > + KeyValuesList *item = g_malloc0(sizeof(*item)); > + item->value = g_malloc0(sizeof(*item->value)); > + item->value->key = g_strdup(qdict_entry_key(entry)); > + item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry)))); > + > + item->next = %(obj)s; > + %(obj)s = item; > + } > +} > +''', > + obj=c_var(argname)) > + else: > + ret += mcgen(''' > %(visitor)s(v,&%(c_name)s, "%(name)s", errp); > ''', > c_name=c_var(argname), name=argname, argtype=argtype, > visitor=type_visitor(argtype)) > - if optional: > + if optional and not '**': > pop_indent() > ret += mcgen(''' > } > diff --git a/scripts/qapi.py b/scripts/qapi.py > index e062336..87b9ee6 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 'KeyValuesList *' > elif type(name) == list: > return '%s *' % c_list_type(name[0]) > elif is_enum(name):
On Thu, 29 Mar 2012 13:26:34 -0500 Anthony Liguori <aliguori@us.ibm.com> wrote: > On 03/29/2012 12:26 PM, Luiz Capitulino wrote: > > This allows for QAPI functions to receive a variable-length argument > > list. This is going to be used by device_add and netdev_add commands. > > > > In the schema, the argument list is represented by type name '**', > > like this example: > > > > { 'command': 'foo', 'data': { 'arg-list': '**' } } > > > > Each argument is represented by the KeyValues type and the C > > implementation should expect a KeyValuesList, like: > > > > void qmp_foo(KeyValuesList *values_list, Error **errp); > > > > XXX: This implementation is simple but very hacky. We just iterate > > through all arguments and build the KeyValuesList list to be > > passed to the QAPI function. > > > > Maybe we could have a kwargs type, that does exactly this but > > through a visitor instead? > > > > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> > > What about just treating '**' as "marshal remaining arguments to a string" and > then pass that string to device_add? qmp_device_add can then parse that string > with QemuOpts. If this turns out to be simple enough, I'm fine with it. > It's a bit ugly, but that's how things worked. When we introduce qom_add, this > problem goes away because you would make multiple calls to qom_set to set all of > the properties. Just out of curiosity, is qom_add going to supersede device_add?
On 03/29/2012 01:42 PM, Luiz Capitulino wrote: > On Thu, 29 Mar 2012 13:26:34 -0500 > Anthony Liguori<aliguori@us.ibm.com> wrote: > >> On 03/29/2012 12:26 PM, Luiz Capitulino wrote: >>> This allows for QAPI functions to receive a variable-length argument >>> list. This is going to be used by device_add and netdev_add commands. >>> >>> In the schema, the argument list is represented by type name '**', >>> like this example: >>> >>> { 'command': 'foo', 'data': { 'arg-list': '**' } } >>> >>> Each argument is represented by the KeyValues type and the C >>> implementation should expect a KeyValuesList, like: >>> >>> void qmp_foo(KeyValuesList *values_list, Error **errp); >>> >>> XXX: This implementation is simple but very hacky. We just iterate >>> through all arguments and build the KeyValuesList list to be >>> passed to the QAPI function. >>> >>> Maybe we could have a kwargs type, that does exactly this but >>> through a visitor instead? >>> >>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> >> >> What about just treating '**' as "marshal remaining arguments to a string" and >> then pass that string to device_add? qmp_device_add can then parse that string >> with QemuOpts. > > If this turns out to be simple enough, I'm fine with it. I don't love doing this sort of double conversion but it's really the only practical way to do it I think. device_add has a weird semantic where printing/parsing is implied so I think it's unavoidable. >> It's a bit ugly, but that's how things worked. When we introduce qom_add, this >> problem goes away because you would make multiple calls to qom_set to set all of >> the properties. > > Just out of curiosity, is qom_add going to supersede device_add? Eventually... I'd like to see qom_add as the low level interface but then I'd like to see nice high level interfaces like 'block_add' which took a parameter of virtio-blk and did all of the magic to create a block device in such a way that's compliant to the current machine type. Regards, Anthony Liguori
On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote: > On 03/29/2012 12:26 PM, Luiz Capitulino wrote: > >This allows for QAPI functions to receive a variable-length argument > >list. This is going to be used by device_add and netdev_add commands. > > > >In the schema, the argument list is represented by type name '**', > >like this example: > > > > { 'command': 'foo', 'data': { 'arg-list': '**' } } > > > >Each argument is represented by the KeyValues type and the C > >implementation should expect a KeyValuesList, like: > > > > void qmp_foo(KeyValuesList *values_list, Error **errp); > > > >XXX: This implementation is simple but very hacky. We just iterate > > through all arguments and build the KeyValuesList list to be > > passed to the QAPI function. > > > > Maybe we could have a kwargs type, that does exactly this but > > through a visitor instead? > > > >Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> > > What about just treating '**' as "marshal remaining arguments to a > string" and then pass that string to device_add? qmp_device_add can > then parse that string with QemuOpts. Since currently we explicitly point qmp to the marshaller anyway, we could also just treat '**' as an indicator to not generate a marshaller. Then, we open-code the marshaller to process the QDict, rather than embedding it in the script or passing it through to qmp_device_add(). From the perspective of qmp_device_add() it then just looks like any other qmp command. > > It's a bit ugly, but that's how things worked. When we introduce > qom_add, this problem goes away because you would make multiple > calls to qom_set to set all of the properties. > > Regards, > > Anthony Liguori > > >--- > > qapi-schema.json | 15 +++++++++++++++ > > scripts/qapi-commands.py | 31 ++++++++++++++++++++++++++++--- > > scripts/qapi.py | 2 ++ > > 3 files changed, 45 insertions(+), 3 deletions(-) > > > >diff --git a/qapi-schema.json b/qapi-schema.json > >index 0d11d6e..25bd487 100644 > >--- a/qapi-schema.json > >+++ b/qapi-schema.json > >@@ -1701,3 +1701,18 @@ > > # Since: 1.1 > > ## > > { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} } > >+ > >+## > >+# @KeyValues: > >+# > >+# A generic representation of a key value pair. > >+# > >+# @key: the name of the item > >+# > >+# @value: the string representation of the item value. This typically follows > >+# QEMU's command line parsing format. See the man pages for more > >+# information. > >+# > >+# Since: 0.14.0 > >+## > >+{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} } > >diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > >index 30a24d2..75a6e81 100644 > >--- a/scripts/qapi-commands.py > >+++ b/scripts/qapi-commands.py > >@@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi); > > obj=obj) > > > > for argname, argtype, optional, structured in parse_args(args): > >- if optional: > >+ if optional and not '**': > > ret += mcgen(''' > > visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp); > > if (has_%(c_name)s) { > > ''', > > c_name=c_var(argname), name=argname) > > push_indent() > >- ret += mcgen(''' > >+ if argtype == '**': > >+ if dealloc: > >+ ret += mcgen(''' > >+qapi_free_KeyValuesList(%(obj)s); > >+''', > >+ obj=c_var(argname)) > >+ else: > >+ ret += mcgen(''' > >+{ > >+ const QDictEntry *entry; > >+ v = v; /* fix me baby */ > >+ > >+ for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) { > >+ KeyValuesList *item = g_malloc0(sizeof(*item)); > >+ item->value = g_malloc0(sizeof(*item->value)); > >+ item->value->key = g_strdup(qdict_entry_key(entry)); > >+ item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry)))); > >+ > >+ item->next = %(obj)s; > >+ %(obj)s = item; > >+ } > >+} > >+''', > >+ obj=c_var(argname)) > >+ else: > >+ ret += mcgen(''' > > %(visitor)s(v,&%(c_name)s, "%(name)s", errp); > > ''', > > c_name=c_var(argname), name=argname, argtype=argtype, > > visitor=type_visitor(argtype)) > >- if optional: > >+ if optional and not '**': > > pop_indent() > > ret += mcgen(''' > > } > >diff --git a/scripts/qapi.py b/scripts/qapi.py > >index e062336..87b9ee6 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 'KeyValuesList *' > > elif type(name) == list: > > return '%s *' % c_list_type(name[0]) > > elif is_enum(name): >
On 03/29/2012 02:28 PM, Michael Roth wrote: > On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote: >> On 03/29/2012 12:26 PM, Luiz Capitulino wrote: >>> This allows for QAPI functions to receive a variable-length argument >>> list. This is going to be used by device_add and netdev_add commands. >>> >>> In the schema, the argument list is represented by type name '**', >>> like this example: >>> >>> { 'command': 'foo', 'data': { 'arg-list': '**' } } >>> >>> Each argument is represented by the KeyValues type and the C >>> implementation should expect a KeyValuesList, like: >>> >>> void qmp_foo(KeyValuesList *values_list, Error **errp); >>> >>> XXX: This implementation is simple but very hacky. We just iterate >>> through all arguments and build the KeyValuesList list to be >>> passed to the QAPI function. >>> >>> Maybe we could have a kwargs type, that does exactly this but >>> through a visitor instead? >>> >>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> >> >> What about just treating '**' as "marshal remaining arguments to a >> string" and then pass that string to device_add? qmp_device_add can >> then parse that string with QemuOpts. > > Since currently we explicitly point qmp to the marshaller anyway, we > could also just treat '**' as an indicator to not generate a marshaller. > Then, we open-code the marshaller to process the QDict, rather than embedding > it in the script or passing it through to qmp_device_add(). You could also just do gen=False... But I don't think open coding the marshaller is the right thing here. You have to convert to strings and reparse anyway. The code needs to be shared between device_add and netdev_add too. Regards, Anthony Liguori > >> From the perspective of qmp_device_add() it then just looks like any > other qmp command. > >> >> It's a bit ugly, but that's how things worked. When we introduce >> qom_add, this problem goes away because you would make multiple >> calls to qom_set to set all of the properties. >> >> Regards, >> >> Anthony Liguori >> >>> --- >>> qapi-schema.json | 15 +++++++++++++++ >>> scripts/qapi-commands.py | 31 ++++++++++++++++++++++++++++--- >>> scripts/qapi.py | 2 ++ >>> 3 files changed, 45 insertions(+), 3 deletions(-) >>> >>> diff --git a/qapi-schema.json b/qapi-schema.json >>> index 0d11d6e..25bd487 100644 >>> --- a/qapi-schema.json >>> +++ b/qapi-schema.json >>> @@ -1701,3 +1701,18 @@ >>> # Since: 1.1 >>> ## >>> { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} } >>> + >>> +## >>> +# @KeyValues: >>> +# >>> +# A generic representation of a key value pair. >>> +# >>> +# @key: the name of the item >>> +# >>> +# @value: the string representation of the item value. This typically follows >>> +# QEMU's command line parsing format. See the man pages for more >>> +# information. >>> +# >>> +# Since: 0.14.0 >>> +## >>> +{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} } >>> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py >>> index 30a24d2..75a6e81 100644 >>> --- a/scripts/qapi-commands.py >>> +++ b/scripts/qapi-commands.py >>> @@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi); >>> obj=obj) >>> >>> for argname, argtype, optional, structured in parse_args(args): >>> - if optional: >>> + if optional and not '**': >>> ret += mcgen(''' >>> visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp); >>> if (has_%(c_name)s) { >>> ''', >>> c_name=c_var(argname), name=argname) >>> push_indent() >>> - ret += mcgen(''' >>> + if argtype == '**': >>> + if dealloc: >>> + ret += mcgen(''' >>> +qapi_free_KeyValuesList(%(obj)s); >>> +''', >>> + obj=c_var(argname)) >>> + else: >>> + ret += mcgen(''' >>> +{ >>> + const QDictEntry *entry; >>> + v = v; /* fix me baby */ >>> + >>> + for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) { >>> + KeyValuesList *item = g_malloc0(sizeof(*item)); >>> + item->value = g_malloc0(sizeof(*item->value)); >>> + item->value->key = g_strdup(qdict_entry_key(entry)); >>> + item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry)))); >>> + >>> + item->next = %(obj)s; >>> + %(obj)s = item; >>> + } >>> +} >>> +''', >>> + obj=c_var(argname)) >>> + else: >>> + ret += mcgen(''' >>> %(visitor)s(v,&%(c_name)s, "%(name)s", errp); >>> ''', >>> c_name=c_var(argname), name=argname, argtype=argtype, >>> visitor=type_visitor(argtype)) >>> - if optional: >>> + if optional and not '**': >>> pop_indent() >>> ret += mcgen(''' >>> } >>> diff --git a/scripts/qapi.py b/scripts/qapi.py >>> index e062336..87b9ee6 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 'KeyValuesList *' >>> elif type(name) == list: >>> return '%s *' % c_list_type(name[0]) >>> elif is_enum(name): >> >
Il 29/03/2012 21:28, Michael Roth ha scritto: > Since currently we explicitly point qmp to the marshaller anyway, we > could also just treat '**' as an indicator to not generate a marshaller. > Then, we open-code the marshaller to process the QDict, rather than embedding > it in the script or passing it through to qmp_device_add(). > > From the perspective of qmp_device_add() it then just looks like any > other qmp command. That's what no_gen does, right? Paolo
On Thu, Mar 29, 2012 at 03:01:16PM -0500, Anthony Liguori wrote: > On 03/29/2012 02:28 PM, Michael Roth wrote: > >On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote: > >>On 03/29/2012 12:26 PM, Luiz Capitulino wrote: > >>>This allows for QAPI functions to receive a variable-length argument > >>>list. This is going to be used by device_add and netdev_add commands. > >>> > >>>In the schema, the argument list is represented by type name '**', > >>>like this example: > >>> > >>> { 'command': 'foo', 'data': { 'arg-list': '**' } } > >>> > >>>Each argument is represented by the KeyValues type and the C > >>>implementation should expect a KeyValuesList, like: > >>> > >>> void qmp_foo(KeyValuesList *values_list, Error **errp); > >>> > >>>XXX: This implementation is simple but very hacky. We just iterate > >>> through all arguments and build the KeyValuesList list to be > >>> passed to the QAPI function. > >>> > >>> Maybe we could have a kwargs type, that does exactly this but > >>> through a visitor instead? > >>> > >>>Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> > >> > >>What about just treating '**' as "marshal remaining arguments to a > >>string" and then pass that string to device_add? qmp_device_add can > >>then parse that string with QemuOpts. > > > >Since currently we explicitly point qmp to the marshaller anyway, we > >could also just treat '**' as an indicator to not generate a marshaller. > >Then, we open-code the marshaller to process the QDict, rather than embedding > >it in the script or passing it through to qmp_device_add(). > > You could also just do gen=False... Ahh, yes we could. Nice :) > > But I don't think open coding the marshaller is the right thing > here. You have to convert to strings and reparse anyway. The code > needs to be shared between device_add and netdev_add too. The only thing the marshallers need to do is call qemu_opts_from_qdict() and pass them on to qdev_device_add()/net_client_init()/etc. We'd basically be taking the current qmp implementations and modifying their call signatures to be compatible with qmp-dispatch.c in the future. It's not really qapi-ish, admittedly, but neither is a built-in varargs sort of type. I'd just prefer not to bake legacy hooks into the code generators if we don't have to. If we absolutely have to do this in the future, it would be more sense to define such fields as being string arguments from the get-go. > > Regards, > > Anthony Liguori > > > > >>From the perspective of qmp_device_add() it then just looks like any > >other qmp command. > > > >> > >>It's a bit ugly, but that's how things worked. When we introduce > >>qom_add, this problem goes away because you would make multiple > >>calls to qom_set to set all of the properties. > >> > >>Regards, > >> > >>Anthony Liguori > >> > >>>--- > >>> qapi-schema.json | 15 +++++++++++++++ > >>> scripts/qapi-commands.py | 31 ++++++++++++++++++++++++++++--- > >>> scripts/qapi.py | 2 ++ > >>> 3 files changed, 45 insertions(+), 3 deletions(-) > >>> > >>>diff --git a/qapi-schema.json b/qapi-schema.json > >>>index 0d11d6e..25bd487 100644 > >>>--- a/qapi-schema.json > >>>+++ b/qapi-schema.json > >>>@@ -1701,3 +1701,18 @@ > >>> # Since: 1.1 > >>> ## > >>> { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} } > >>>+ > >>>+## > >>>+# @KeyValues: > >>>+# > >>>+# A generic representation of a key value pair. > >>>+# > >>>+# @key: the name of the item > >>>+# > >>>+# @value: the string representation of the item value. This typically follows > >>>+# QEMU's command line parsing format. See the man pages for more > >>>+# information. > >>>+# > >>>+# Since: 0.14.0 > >>>+## > >>>+{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} } > >>>diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > >>>index 30a24d2..75a6e81 100644 > >>>--- a/scripts/qapi-commands.py > >>>+++ b/scripts/qapi-commands.py > >>>@@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi); > >>> obj=obj) > >>> > >>> for argname, argtype, optional, structured in parse_args(args): > >>>- if optional: > >>>+ if optional and not '**': > >>> ret += mcgen(''' > >>> visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp); > >>> if (has_%(c_name)s) { > >>> ''', > >>> c_name=c_var(argname), name=argname) > >>> push_indent() > >>>- ret += mcgen(''' > >>>+ if argtype == '**': > >>>+ if dealloc: > >>>+ ret += mcgen(''' > >>>+qapi_free_KeyValuesList(%(obj)s); > >>>+''', > >>>+ obj=c_var(argname)) > >>>+ else: > >>>+ ret += mcgen(''' > >>>+{ > >>>+ const QDictEntry *entry; > >>>+ v = v; /* fix me baby */ > >>>+ > >>>+ for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) { > >>>+ KeyValuesList *item = g_malloc0(sizeof(*item)); > >>>+ item->value = g_malloc0(sizeof(*item->value)); > >>>+ item->value->key = g_strdup(qdict_entry_key(entry)); > >>>+ item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry)))); > >>>+ > >>>+ item->next = %(obj)s; > >>>+ %(obj)s = item; > >>>+ } > >>>+} > >>>+''', > >>>+ obj=c_var(argname)) > >>>+ else: > >>>+ ret += mcgen(''' > >>> %(visitor)s(v,&%(c_name)s, "%(name)s", errp); > >>> ''', > >>> c_name=c_var(argname), name=argname, argtype=argtype, > >>> visitor=type_visitor(argtype)) > >>>- if optional: > >>>+ if optional and not '**': > >>> pop_indent() > >>> ret += mcgen(''' > >>> } > >>>diff --git a/scripts/qapi.py b/scripts/qapi.py > >>>index e062336..87b9ee6 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 'KeyValuesList *' > >>> elif type(name) == list: > >>> return '%s *' % c_list_type(name[0]) > >>> elif is_enum(name): > >> > > >
On Thu, Mar 29, 2012 at 05:39:10PM -0500, Michael Roth wrote: > On Thu, Mar 29, 2012 at 03:01:16PM -0500, Anthony Liguori wrote: > > On 03/29/2012 02:28 PM, Michael Roth wrote: > > >On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote: > > >>On 03/29/2012 12:26 PM, Luiz Capitulino wrote: > > >>>This allows for QAPI functions to receive a variable-length argument > > >>>list. This is going to be used by device_add and netdev_add commands. > > >>> > > >>>In the schema, the argument list is represented by type name '**', > > >>>like this example: > > >>> > > >>> { 'command': 'foo', 'data': { 'arg-list': '**' } } > > >>> > > >>>Each argument is represented by the KeyValues type and the C > > >>>implementation should expect a KeyValuesList, like: > > >>> > > >>> void qmp_foo(KeyValuesList *values_list, Error **errp); > > >>> > > >>>XXX: This implementation is simple but very hacky. We just iterate > > >>> through all arguments and build the KeyValuesList list to be > > >>> passed to the QAPI function. > > >>> > > >>> Maybe we could have a kwargs type, that does exactly this but > > >>> through a visitor instead? > > >>> > > >>>Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> > > >> > > >>What about just treating '**' as "marshal remaining arguments to a > > >>string" and then pass that string to device_add? qmp_device_add can > > >>then parse that string with QemuOpts. > > > > > >Since currently we explicitly point qmp to the marshaller anyway, we > > >could also just treat '**' as an indicator to not generate a marshaller. > > >Then, we open-code the marshaller to process the QDict, rather than embedding > > >it in the script or passing it through to qmp_device_add(). > > > > You could also just do gen=False... > > Ahh, yes we could. Nice :) > > > > > But I don't think open coding the marshaller is the right thing > > here. You have to convert to strings and reparse anyway. The code > > needs to be shared between device_add and netdev_add too. > > The only thing the marshallers need to do is call qemu_opts_from_qdict() > and pass them on to qdev_device_add()/net_client_init()/etc. We'd basically > be taking the current qmp implementations and modifying their call signatures > to be compatible with qmp-dispatch.c in the future. It's not really Err, scratch that... I've been living in qemu-ga land too long. For QMP this basically amounts to a no-op for now (other than documenting the command in the schema and doing the error-handling cleanups), and when we switch to the new qmp server/dispatch stuff we just massage the function signature to match the marshaller qapi expects and continue to skip marshaller generation. > qapi-ish, admittedly, but neither is a built-in varargs sort of type. > > I'd just prefer not to bake legacy hooks into the code generators if we > don't have to. If we absolutely have to do this in the future, it would be more > sense to define such fields as being string arguments from the get-go. > > > > > Regards, > > > > Anthony Liguori > > > > > > > >>From the perspective of qmp_device_add() it then just looks like any > > >other qmp command. > > > > > >> > > >>It's a bit ugly, but that's how things worked. When we introduce > > >>qom_add, this problem goes away because you would make multiple > > >>calls to qom_set to set all of the properties. > > >> > > >>Regards, > > >> > > >>Anthony Liguori > > >> > > >>>--- > > >>> qapi-schema.json | 15 +++++++++++++++ > > >>> scripts/qapi-commands.py | 31 ++++++++++++++++++++++++++++--- > > >>> scripts/qapi.py | 2 ++ > > >>> 3 files changed, 45 insertions(+), 3 deletions(-) > > >>> > > >>>diff --git a/qapi-schema.json b/qapi-schema.json > > >>>index 0d11d6e..25bd487 100644 > > >>>--- a/qapi-schema.json > > >>>+++ b/qapi-schema.json > > >>>@@ -1701,3 +1701,18 @@ > > >>> # Since: 1.1 > > >>> ## > > >>> { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} } > > >>>+ > > >>>+## > > >>>+# @KeyValues: > > >>>+# > > >>>+# A generic representation of a key value pair. > > >>>+# > > >>>+# @key: the name of the item > > >>>+# > > >>>+# @value: the string representation of the item value. This typically follows > > >>>+# QEMU's command line parsing format. See the man pages for more > > >>>+# information. > > >>>+# > > >>>+# Since: 0.14.0 > > >>>+## > > >>>+{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} } > > >>>diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > > >>>index 30a24d2..75a6e81 100644 > > >>>--- a/scripts/qapi-commands.py > > >>>+++ b/scripts/qapi-commands.py > > >>>@@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi); > > >>> obj=obj) > > >>> > > >>> for argname, argtype, optional, structured in parse_args(args): > > >>>- if optional: > > >>>+ if optional and not '**': > > >>> ret += mcgen(''' > > >>> visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp); > > >>> if (has_%(c_name)s) { > > >>> ''', > > >>> c_name=c_var(argname), name=argname) > > >>> push_indent() > > >>>- ret += mcgen(''' > > >>>+ if argtype == '**': > > >>>+ if dealloc: > > >>>+ ret += mcgen(''' > > >>>+qapi_free_KeyValuesList(%(obj)s); > > >>>+''', > > >>>+ obj=c_var(argname)) > > >>>+ else: > > >>>+ ret += mcgen(''' > > >>>+{ > > >>>+ const QDictEntry *entry; > > >>>+ v = v; /* fix me baby */ > > >>>+ > > >>>+ for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) { > > >>>+ KeyValuesList *item = g_malloc0(sizeof(*item)); > > >>>+ item->value = g_malloc0(sizeof(*item->value)); > > >>>+ item->value->key = g_strdup(qdict_entry_key(entry)); > > >>>+ item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry)))); > > >>>+ > > >>>+ item->next = %(obj)s; > > >>>+ %(obj)s = item; > > >>>+ } > > >>>+} > > >>>+''', > > >>>+ obj=c_var(argname)) > > >>>+ else: > > >>>+ ret += mcgen(''' > > >>> %(visitor)s(v,&%(c_name)s, "%(name)s", errp); > > >>> ''', > > >>> c_name=c_var(argname), name=argname, argtype=argtype, > > >>> visitor=type_visitor(argtype)) > > >>>- if optional: > > >>>+ if optional and not '**': > > >>> pop_indent() > > >>> ret += mcgen(''' > > >>> } > > >>>diff --git a/scripts/qapi.py b/scripts/qapi.py > > >>>index e062336..87b9ee6 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 'KeyValuesList *' > > >>> elif type(name) == list: > > >>> return '%s *' % c_list_type(name[0]) > > >>> elif is_enum(name): > > >> > > > > >
Il 29/03/2012 22:01, Anthony Liguori ha scritto: >> Then, we open-code the marshaller to process the QDict, rather than >> embedding >> it in the script or passing it through to qmp_device_add(). > > You could also just do gen=False... > > But I don't think open coding the marshaller is the right thing here. > You have to convert to strings and reparse anyway. device_add expects strings, you don't have to convert no? There is a single parse point. (Instead, a qom_add would accept the right types, but then it would also use a more QAPI-friendly syntax with lists insteads of varargs). Regarding device_add ? and device_add foo,? I would implement it as separate QMP commands hooking into QOM, such as qom_list_types (taking the superclass as an optional argument) and qom_properties. But the latter first needs static properties to move up from devices to objects. I'll take a look at that. Paolo
On Fri, 30 Mar 2012 09:49:20 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Regarding device_add ? and device_add foo,? I would implement it as > separate QMP commands hooking into QOM, such as qom_list_types (taking > the superclass as an optional argument) and qom_properties. But the > latter first needs static properties to move up from devices to objects. Yes, after I suggested query-devices I remembered about qom_list_types. I think it's ok for hmp.c to use it even if it's not declared stable yet, right? Anthony, you said you did it in your tree differently, did you? I think using qom_list_types is the way to go here...
On Fri, 30 Mar 2012 09:49:20 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Regarding device_add ? and device_add foo,? I would implement it as > separate QMP commands hooking into QOM, such as qom_list_types (taking > the superclass as an optional argument) and qom_properties. But the > latter first needs static properties to move up from devices to objects. > I'll take a look at that. Have you? I'm resuming this topic because I've resumed my work on this series. I think that for device_add foo,? we could introduce qom-list-type-properties. Seems simple if limited to devices, not sure how to make it generic though. But it shouldn't be a problem to limit it to devices at this point, as all qom commands are unstable, right? Now, I have no idea how to make device_add ?, as it accesses DeviceClass properties like no_user and bus_info...
Il 10/04/2012 16:23, Luiz Capitulino ha scritto: >> > Regarding device_add ? and device_add foo,? I would implement it as >> > separate QMP commands hooking into QOM, such as qom_list_types (taking >> > the superclass as an optional argument) and qom_properties. But the >> > latter first needs static properties to move up from devices to objects. >> > I'll take a look at that. > Have you? Yes, patches were on the list. I'll submit v2 tomorrow or thursday. > I think that for device_add foo,? we could introduce qom-list-type-properties. > Seems simple if limited to devices, not sure how to make it generic though. With those patches you can make it generic. Paolo
On Tue, 10 Apr 2012 16:27:43 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 10/04/2012 16:23, Luiz Capitulino ha scritto: > >> > Regarding device_add ? and device_add foo,? I would implement it as > >> > separate QMP commands hooking into QOM, such as qom_list_types (taking > >> > the superclass as an optional argument) and qom_properties. But the > >> > latter first needs static properties to move up from devices to objects. > >> > I'll take a look at that. > > Have you? > > Yes, patches were on the list. I'll submit v2 tomorrow or thursday. Great. > > I think that for device_add foo,? we could introduce qom-list-type-properties. > > Seems simple if limited to devices, not sure how to make it generic though. > > With those patches you can make it generic. Cool, what about device_add ?, any recommendations?
Il 10/04/2012 16:30, Luiz Capitulino ha scritto: > On Tue, 10 Apr 2012 16:27:43 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Il 10/04/2012 16:23, Luiz Capitulino ha scritto: >>>>> Regarding device_add ? and device_add foo,? I would implement it as >>>>> separate QMP commands hooking into QOM, such as qom_list_types (taking >>>>> the superclass as an optional argument) and qom_properties. But the >>>>> latter first needs static properties to move up from devices to objects. >>>>> I'll take a look at that. >>> Have you? >> >> Yes, patches were on the list. I'll submit v2 tomorrow or thursday. > > Great. > >>> I think that for device_add foo,? we could introduce qom-list-type-properties. >>> Seems simple if limited to devices, not sure how to make it generic though. >> >> With those patches you can make it generic. > > Cool, what about device_add ?, any recommendations? That's easier, you can just list non-abstract types with TYPE_DEVICE superclass (note that's a macro). Paolo
On Tue, 10 Apr 2012 18:40:25 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 10/04/2012 16:30, Luiz Capitulino ha scritto: > > On Tue, 10 Apr 2012 16:27:43 +0200 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> Il 10/04/2012 16:23, Luiz Capitulino ha scritto: > >>>>> Regarding device_add ? and device_add foo,? I would implement it as > >>>>> separate QMP commands hooking into QOM, such as qom_list_types (taking > >>>>> the superclass as an optional argument) and qom_properties. But the > >>>>> latter first needs static properties to move up from devices to objects. > >>>>> I'll take a look at that. > >>> Have you? > >> > >> Yes, patches were on the list. I'll submit v2 tomorrow or thursday. > > > > Great. > > > >>> I think that for device_add foo,? we could introduce qom-list-type-properties. > >>> Seems simple if limited to devices, not sure how to make it generic though. > >> > >> With those patches you can make it generic. > > > > Cool, what about device_add ?, any recommendations? > > That's easier, you can just list non-abstract types with TYPE_DEVICE > superclass (note that's a macro). By using qom-list-types? The problem is how to export fields like 'no_user' in QMP.
Il 10/04/2012 18:46, Luiz Capitulino ha scritto: >>>>> I think that for device_add foo,? we could introduce qom-list-type-properties. >>>>> Seems simple if limited to devices, not sure how to make it generic though. >>>> >>>> With those patches you can make it generic. >>> >>> Cool, what about device_add ?, any recommendations? >> >> That's easier, you can just list non-abstract types with TYPE_DEVICE >> superclass (note that's a macro). > > By using qom-list-types? The problem is how to export fields like 'no_user' > in QMP. You're right... Anthony, wdyt? Paolo
diff --git a/qapi-schema.json b/qapi-schema.json index 0d11d6e..25bd487 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1701,3 +1701,18 @@ # Since: 1.1 ## { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} } + +## +# @KeyValues: +# +# A generic representation of a key value pair. +# +# @key: the name of the item +# +# @value: the string representation of the item value. This typically follows +# QEMU's command line parsing format. See the man pages for more +# information. +# +# Since: 0.14.0 +## +{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} } diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 30a24d2..75a6e81 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi); obj=obj) for argname, argtype, optional, structured in parse_args(args): - if optional: + if optional and not '**': ret += mcgen(''' visit_start_optional(v, &has_%(c_name)s, "%(name)s", errp); if (has_%(c_name)s) { ''', c_name=c_var(argname), name=argname) push_indent() - ret += mcgen(''' + if argtype == '**': + if dealloc: + ret += mcgen(''' +qapi_free_KeyValuesList(%(obj)s); +''', + obj=c_var(argname)) + else: + ret += mcgen(''' +{ + const QDictEntry *entry; + v = v; /* fix me baby */ + + for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) { + KeyValuesList *item = g_malloc0(sizeof(*item)); + item->value = g_malloc0(sizeof(*item->value)); + item->value->key = g_strdup(qdict_entry_key(entry)); + item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry)))); + + item->next = %(obj)s; + %(obj)s = item; + } +} +''', + obj=c_var(argname)) + else: + ret += mcgen(''' %(visitor)s(v, &%(c_name)s, "%(name)s", errp); ''', c_name=c_var(argname), name=argname, argtype=argtype, visitor=type_visitor(argtype)) - if optional: + if optional and not '**': pop_indent() ret += mcgen(''' } diff --git a/scripts/qapi.py b/scripts/qapi.py index e062336..87b9ee6 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 'KeyValuesList *' elif type(name) == list: return '%s *' % c_list_type(name[0]) elif is_enum(name):
This allows for QAPI functions to receive a variable-length argument list. This is going to be used by device_add and netdev_add commands. In the schema, the argument list is represented by type name '**', like this example: { 'command': 'foo', 'data': { 'arg-list': '**' } } Each argument is represented by the KeyValues type and the C implementation should expect a KeyValuesList, like: void qmp_foo(KeyValuesList *values_list, Error **errp); XXX: This implementation is simple but very hacky. We just iterate through all arguments and build the KeyValuesList list to be passed to the QAPI function. Maybe we could have a kwargs type, that does exactly this but through a visitor instead? Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- qapi-schema.json | 15 +++++++++++++++ scripts/qapi-commands.py | 31 ++++++++++++++++++++++++++++--- scripts/qapi.py | 2 ++ 3 files changed, 45 insertions(+), 3 deletions(-)