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