Re: [PATCH v3 27/30] hmp: QAPIfy object_add
* Kevin Wolf (kw...@redhat.com) wrote:
> Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
> > Paolo Bonzini writes:
> >
> > > On 13/03/21 14:28, Markus Armbruster wrote:
> > >> Kevin Wolf writes:
> > >>
> > >>> This switches the HMP command object_add from a QemuOpts-based parser to
> > >>> user_creatable_add_from_str() which uses a keyval parser and enforces
> > >>> the QAPI schema.
> > >>>
> > >>> Apart from being a cleanup, this makes non-scalar properties and help
> > >>> accessible. In order for help to be printed to the monitor instead of
> > >>> stdout, the printf() calls in the help functions are changed to
> > >>> qemu_printf().
> > >>>
> > >>> Signed-off-by: Kevin Wolf
> > >>> Acked-by: Peter Krempa
> > >>> Reviewed-by: Eric Blake
> > >>> Reviewed-by: Dr. David Alan Gilbert
> > >>> ---
> > >>> monitor/hmp-cmds.c | 17 ++---
> > >>> qom/object_interfaces.c | 11 ++-
> > >>> hmp-commands.hx | 2 +-
> > >>> 3 files changed, 9 insertions(+), 21 deletions(-)
> > >>>
> > >>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > >>> index 3c88a4faef..652cf9ff21 100644
> > >>> --- a/monitor/hmp-cmds.c
> > >>> +++ b/monitor/hmp-cmds.c
> > >>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict
> > >>> *qdict)
> > >>>
> > >>> void hmp_object_add(Monitor *mon, const QDict *qdict)
> > >>> {
> > >>> +const char *options = qdict_get_str(qdict, "object");
> > >>> Error *err = NULL;
> > >>> -QemuOpts *opts;
> > >>> -Object *obj = NULL;
> > >>> -
> > >>> -opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> > >>> -if (err) {
> > >>> -goto end;
> > >>> -}
> > >>>
> > >>> -obj = user_creatable_add_opts(opts, &err);
> > >>> -qemu_opts_del(opts);
> > >>> -
> > >>> -end:
> > >>> +user_creatable_add_from_str(options, &err);
> > >>> hmp_handle_error(mon, err);
> > >>> -
> > >>> -if (obj) {
> > >>> -object_unref(obj);
> > >>> -}
> > >>> }
> > >>
> > >> Doesn't this break the list-valued properties (Memdev member host-nodes,
> > >> NumaNodeOptions member cpus) exactly the same way that made us keep
> > >> QemuOpts for qemu-system-FOO -object?
> > >
> > > Yes, it does. I guess it can just be documented, unlike for the command
> > > line?
> >
> > Maybe. Judgement call, not mine to make.
> >
> > Do people create such objects in HMP? I figure we don't really know.
> > Educated guess?
> >
> > If you try, how does it break? Is it confusing? Can you show an
> > example?
>
> (qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=0
> Error: Invalid parameter type for 'host-nodes', expected: array
> (qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes.0=0
> (qemu)
>
> HMP is not a stable interface, so changing the syntax didn't feel like a
> problem to me. I doubt many people do HMP memory hotplug while setting a
> specific NUMA policy, but it wouldn't change my assessment anyway. I
> should have made this explicit in the commit message, though.
I'm OK for it to change, but yes I'd like to have the before/after
syntax listed somewhere as easy references for people confused.
Dave
> Kevin
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v3 27/30] hmp: QAPIfy object_add
On 15/03/21 12:38, Dr. David Alan Gilbert wrote:
* Kevin Wolf (kw...@redhat.com) wrote:
Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
Paolo Bonzini writes:
On 13/03/21 14:28, Markus Armbruster wrote:
Kevin Wolf writes:
This switches the HMP command object_add from a QemuOpts-based parser to
user_creatable_add_from_str() which uses a keyval parser and enforces
the QAPI schema.
Apart from being a cleanup, this makes non-scalar properties and help
accessible. In order for help to be printed to the monitor instead of
stdout, the printf() calls in the help functions are changed to
qemu_printf().
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
Reviewed-by: Dr. David Alan Gilbert
---
monitor/hmp-cmds.c | 17 ++---
qom/object_interfaces.c | 11 ++-
hmp-commands.hx | 2 +-
3 files changed, 9 insertions(+), 21 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 3c88a4faef..652cf9ff21 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
void hmp_object_add(Monitor *mon, const QDict *qdict)
{
+const char *options = qdict_get_str(qdict, "object");
Error *err = NULL;
-QemuOpts *opts;
-Object *obj = NULL;
-
-opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
-if (err) {
-goto end;
-}
-obj = user_creatable_add_opts(opts, &err);
-qemu_opts_del(opts);
-
-end:
+user_creatable_add_from_str(options, &err);
hmp_handle_error(mon, err);
-
-if (obj) {
-object_unref(obj);
-}
}
Doesn't this break the list-valued properties (Memdev member host-nodes,
NumaNodeOptions member cpus) exactly the same way that made us keep
QemuOpts for qemu-system-FOO -object?
Yes, it does. I guess it can just be documented, unlike for the command
line?
Maybe. Judgement call, not mine to make.
Do people create such objects in HMP? I figure we don't really know.
Educated guess?
If you try, how does it break? Is it confusing? Can you show an
example?
(qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=0
Error: Invalid parameter type for 'host-nodes', expected: array
(qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes.0=0
(qemu)
HMP is not a stable interface, so changing the syntax didn't feel like a
problem to me. I doubt many people do HMP memory hotplug while setting a
specific NUMA policy, but it wouldn't change my assessment anyway. I
should have made this explicit in the commit message, though.
I'm OK for it to change, but yes I'd like to have the before/after
syntax listed somewhere as easy references for people confused.
I think we should try to improve the string-value QObject visitor to
also allow JSON values in some places, for example to allow
object_add
memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=[0,1,2,3]
Paolo
Re: [PATCH v3 27/30] hmp: QAPIfy object_add
Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
> Paolo Bonzini writes:
>
> > On 13/03/21 14:28, Markus Armbruster wrote:
> >> Kevin Wolf writes:
> >>
> >>> This switches the HMP command object_add from a QemuOpts-based parser to
> >>> user_creatable_add_from_str() which uses a keyval parser and enforces
> >>> the QAPI schema.
> >>>
> >>> Apart from being a cleanup, this makes non-scalar properties and help
> >>> accessible. In order for help to be printed to the monitor instead of
> >>> stdout, the printf() calls in the help functions are changed to
> >>> qemu_printf().
> >>>
> >>> Signed-off-by: Kevin Wolf
> >>> Acked-by: Peter Krempa
> >>> Reviewed-by: Eric Blake
> >>> Reviewed-by: Dr. David Alan Gilbert
> >>> ---
> >>> monitor/hmp-cmds.c | 17 ++---
> >>> qom/object_interfaces.c | 11 ++-
> >>> hmp-commands.hx | 2 +-
> >>> 3 files changed, 9 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> >>> index 3c88a4faef..652cf9ff21 100644
> >>> --- a/monitor/hmp-cmds.c
> >>> +++ b/monitor/hmp-cmds.c
> >>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict
> >>> *qdict)
> >>>
> >>> void hmp_object_add(Monitor *mon, const QDict *qdict)
> >>> {
> >>> +const char *options = qdict_get_str(qdict, "object");
> >>> Error *err = NULL;
> >>> -QemuOpts *opts;
> >>> -Object *obj = NULL;
> >>> -
> >>> -opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> >>> -if (err) {
> >>> -goto end;
> >>> -}
> >>>
> >>> -obj = user_creatable_add_opts(opts, &err);
> >>> -qemu_opts_del(opts);
> >>> -
> >>> -end:
> >>> +user_creatable_add_from_str(options, &err);
> >>> hmp_handle_error(mon, err);
> >>> -
> >>> -if (obj) {
> >>> -object_unref(obj);
> >>> -}
> >>> }
> >>
> >> Doesn't this break the list-valued properties (Memdev member host-nodes,
> >> NumaNodeOptions member cpus) exactly the same way that made us keep
> >> QemuOpts for qemu-system-FOO -object?
> >
> > Yes, it does. I guess it can just be documented, unlike for the command
> > line?
>
> Maybe. Judgement call, not mine to make.
>
> Do people create such objects in HMP? I figure we don't really know.
> Educated guess?
>
> If you try, how does it break? Is it confusing? Can you show an
> example?
(qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=0
Error: Invalid parameter type for 'host-nodes', expected: array
(qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes.0=0
(qemu)
HMP is not a stable interface, so changing the syntax didn't feel like a
problem to me. I doubt many people do HMP memory hotplug while setting a
specific NUMA policy, but it wouldn't change my assessment anyway. I
should have made this explicit in the commit message, though.
Kevin
Re: [PATCH v3 27/30] hmp: QAPIfy object_add
Paolo Bonzini writes:
> On 13/03/21 14:28, Markus Armbruster wrote:
>> Kevin Wolf writes:
>>
>>> This switches the HMP command object_add from a QemuOpts-based parser to
>>> user_creatable_add_from_str() which uses a keyval parser and enforces
>>> the QAPI schema.
>>>
>>> Apart from being a cleanup, this makes non-scalar properties and help
>>> accessible. In order for help to be printed to the monitor instead of
>>> stdout, the printf() calls in the help functions are changed to
>>> qemu_printf().
>>>
>>> Signed-off-by: Kevin Wolf
>>> Acked-by: Peter Krempa
>>> Reviewed-by: Eric Blake
>>> Reviewed-by: Dr. David Alan Gilbert
>>> ---
>>> monitor/hmp-cmds.c | 17 ++---
>>> qom/object_interfaces.c | 11 ++-
>>> hmp-commands.hx | 2 +-
>>> 3 files changed, 9 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> index 3c88a4faef..652cf9ff21 100644
>>> --- a/monitor/hmp-cmds.c
>>> +++ b/monitor/hmp-cmds.c
>>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict
>>> *qdict)
>>>
>>> void hmp_object_add(Monitor *mon, const QDict *qdict)
>>> {
>>> +const char *options = qdict_get_str(qdict, "object");
>>> Error *err = NULL;
>>> -QemuOpts *opts;
>>> -Object *obj = NULL;
>>> -
>>> -opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
>>> -if (err) {
>>> -goto end;
>>> -}
>>>
>>> -obj = user_creatable_add_opts(opts, &err);
>>> -qemu_opts_del(opts);
>>> -
>>> -end:
>>> +user_creatable_add_from_str(options, &err);
>>> hmp_handle_error(mon, err);
>>> -
>>> -if (obj) {
>>> -object_unref(obj);
>>> -}
>>> }
>>
>> Doesn't this break the list-valued properties (Memdev member host-nodes,
>> NumaNodeOptions member cpus) exactly the same way that made us keep
>> QemuOpts for qemu-system-FOO -object?
>
> Yes, it does. I guess it can just be documented, unlike for the command
> line?
Maybe. Judgement call, not mine to make.
Do people create such objects in HMP? I figure we don't really know.
Educated guess?
If you try, how does it break? Is it confusing? Can you show an
example?
Re: [PATCH v3 27/30] hmp: QAPIfy object_add
On 13/03/21 14:28, Markus Armbruster wrote:
Kevin Wolf writes:
This switches the HMP command object_add from a QemuOpts-based parser to
user_creatable_add_from_str() which uses a keyval parser and enforces
the QAPI schema.
Apart from being a cleanup, this makes non-scalar properties and help
accessible. In order for help to be printed to the monitor instead of
stdout, the printf() calls in the help functions are changed to
qemu_printf().
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
Reviewed-by: Dr. David Alan Gilbert
---
monitor/hmp-cmds.c | 17 ++---
qom/object_interfaces.c | 11 ++-
hmp-commands.hx | 2 +-
3 files changed, 9 insertions(+), 21 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 3c88a4faef..652cf9ff21 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
void hmp_object_add(Monitor *mon, const QDict *qdict)
{
+const char *options = qdict_get_str(qdict, "object");
Error *err = NULL;
-QemuOpts *opts;
-Object *obj = NULL;
-
-opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
-if (err) {
-goto end;
-}
-obj = user_creatable_add_opts(opts, &err);
-qemu_opts_del(opts);
-
-end:
+user_creatable_add_from_str(options, &err);
hmp_handle_error(mon, err);
-
-if (obj) {
-object_unref(obj);
-}
}
Doesn't this break the list-valued properties (Memdev member host-nodes,
NumaNodeOptions member cpus) exactly the same way that made us keep
QemuOpts for qemu-system-FOO -object?
Yes, it does. I guess it can just be documented, unlike for the command
line?
Paolo
Re: [PATCH v3 27/30] hmp: QAPIfy object_add
Kevin Wolf writes:
> This switches the HMP command object_add from a QemuOpts-based parser to
> user_creatable_add_from_str() which uses a keyval parser and enforces
> the QAPI schema.
>
> Apart from being a cleanup, this makes non-scalar properties and help
> accessible. In order for help to be printed to the monitor instead of
> stdout, the printf() calls in the help functions are changed to
> qemu_printf().
>
> Signed-off-by: Kevin Wolf
> Acked-by: Peter Krempa
> Reviewed-by: Eric Blake
> Reviewed-by: Dr. David Alan Gilbert
> ---
> monitor/hmp-cmds.c | 17 ++---
> qom/object_interfaces.c | 11 ++-
> hmp-commands.hx | 2 +-
> 3 files changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 3c88a4faef..652cf9ff21 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>
> void hmp_object_add(Monitor *mon, const QDict *qdict)
> {
> +const char *options = qdict_get_str(qdict, "object");
> Error *err = NULL;
> -QemuOpts *opts;
> -Object *obj = NULL;
> -
> -opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> -if (err) {
> -goto end;
> -}
>
> -obj = user_creatable_add_opts(opts, &err);
> -qemu_opts_del(opts);
> -
> -end:
> +user_creatable_add_from_str(options, &err);
> hmp_handle_error(mon, err);
> -
> -if (obj) {
> -object_unref(obj);
> -}
> }
Doesn't this break the list-valued properties (Memdev member host-nodes,
NumaNodeOptions member cpus) exactly the same way that made us keep
QemuOpts for qemu-system-FOO -object?
[...]
[PATCH v3 27/30] hmp: QAPIfy object_add
This switches the HMP command object_add from a QemuOpts-based parser to
user_creatable_add_from_str() which uses a keyval parser and enforces
the QAPI schema.
Apart from being a cleanup, this makes non-scalar properties and help
accessible. In order for help to be printed to the monitor instead of
stdout, the printf() calls in the help functions are changed to
qemu_printf().
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
Reviewed-by: Dr. David Alan Gilbert
---
monitor/hmp-cmds.c | 17 ++---
qom/object_interfaces.c | 11 ++-
hmp-commands.hx | 2 +-
3 files changed, 9 insertions(+), 21 deletions(-)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 3c88a4faef..652cf9ff21 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
void hmp_object_add(Monitor *mon, const QDict *qdict)
{
+const char *options = qdict_get_str(qdict, "object");
Error *err = NULL;
-QemuOpts *opts;
-Object *obj = NULL;
-
-opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
-if (err) {
-goto end;
-}
-obj = user_creatable_add_opts(opts, &err);
-qemu_opts_del(opts);
-
-end:
+user_creatable_add_from_str(options, &err);
hmp_handle_error(mon, err);
-
-if (obj) {
-object_unref(obj);
-}
}
void hmp_getfd(Monitor *mon, const QDict *qdict)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index bf9f8cd2c6..6dcab60f09 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -14,6 +14,7 @@
#include "qemu/id.h"
#include "qemu/module.h"
#include "qemu/option.h"
+#include "qemu/qemu-print.h"
#include "qapi/opts-visitor.h"
#include "qemu/config-file.h"
@@ -221,11 +222,11 @@ static void user_creatable_print_types(void)
{
GSList *l, *list;
-printf("List of user creatable objects:\n");
+qemu_printf("List of user creatable objects:\n");
list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
for (l = list; l != NULL; l = l->next) {
ObjectClass *oc = OBJECT_CLASS(l->data);
-printf(" %s\n", object_class_get_name(oc));
+qemu_printf(" %s\n", object_class_get_name(oc));
}
g_slist_free(list);
}
@@ -256,12 +257,12 @@ static bool user_creatable_print_type_properites(const
char *type)
}
g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
if (array->len > 0) {
-printf("%s options:\n", type);
+qemu_printf("%s options:\n", type);
} else {
-printf("There are no options for %s.\n", type);
+qemu_printf("There are no options for %s.\n", type);
}
for (i = 0; i < array->len; i++) {
-printf("%s\n", (char *)array->pdata[i]);
+qemu_printf("%s\n", (char *)array->pdata[i]);
}
g_ptr_array_set_free_func(array, g_free);
g_ptr_array_free(array, true);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..6f5d9ce2fb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1337,7 +1337,7 @@ ERST
{
.name = "object_add",
-.args_type = "object:O",
+.args_type = "object:S",
.params = "[qom-type=]type,id=str[,prop=value][,...]",
.help = "create QOM object",
.cmd= hmp_object_add,
--
2.29.2
