[v3,27/30] hmp: QAPIfy object_add

Commit Message

Kevin Wolf March 8, 2021, 4:54 p.m. UTC

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 <[e-mail]>
Acked-by: Peter Krempa <[e-mail]>
Reviewed-by: Eric Blake <[e-mail]>
Reviewed-by: Dr. David Alan Gilbert <[e-mail]>
---
 monitor/hmp-cmds.c      | 17 ++---------------
 qom/object_interfaces.c | 11 ++++++-----
 hmp-commands.hx         |  2 +-
 3 files changed, 9 insertions(+), 21 deletions(-)

Comments

Markus Armbruster March 13, 2021, 1:28 p.m. UTC | #1

Kevin Wolf <[e-mail]> 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 <[e-mail]>
> Acked-by: Peter Krempa <[e-mail]>
> Reviewed-by: Eric Blake <[e-mail]>
> Reviewed-by: Dr. David Alan Gilbert <[e-mail]>
> ---
>  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?

[...]

Paolo Bonzini March 13, 2021, 2:11 p.m. UTC | #2

On 13/03/21 14:28, Markus Armbruster wrote:
> Kevin Wolf <[e-mail]> 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 <[e-mail]>
>> Acked-by: Peter Krempa <[e-mail]>
>> Reviewed-by: Eric Blake <[e-mail]>
>> Reviewed-by: Dr. David Alan Gilbert <[e-mail]>
>> ---
>>   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

Markus Armbruster March 15, 2021, 9:39 a.m. UTC | #3

Paolo Bonzini <[e-mail]> writes:

> On 13/03/21 14:28, Markus Armbruster wrote:
>> Kevin Wolf <[e-mail]> 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 <[e-mail]>
>>> Acked-by: Peter Krempa <[e-mail]>
>>> Reviewed-by: Eric Blake <[e-mail]>
>>> Reviewed-by: Dr. David Alan Gilbert <[e-mail]>
>>> ---
>>>   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?

Kevin Wolf March 15, 2021, 11:09 a.m. UTC | #4

Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
> Paolo Bonzini <[e-mail]> writes:
> 
> > On 13/03/21 14:28, Markus Armbruster wrote:
> >> Kevin Wolf <[e-mail]> 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 <[e-mail]>
> >>> Acked-by: Peter Krempa <[e-mail]>
> >>> Reviewed-by: Eric Blake <[e-mail]>
> >>> Reviewed-by: Dr. David Alan Gilbert <[e-mail]>
> >>> ---
> >>>   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

Dr. David Alan Gilbert March 15, 2021, 11:38 a.m. UTC | #5

* Kevin Wolf ([e-mail]) wrote:
> Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
> > Paolo Bonzini <[e-mail]> writes:
> > 
> > > On 13/03/21 14:28, Markus Armbruster wrote:
> > >> Kevin Wolf <[e-mail]> 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 <[e-mail]>
> > >>> Acked-by: Peter Krempa <[e-mail]>
> > >>> Reviewed-by: Eric Blake <[e-mail]>
> > >>> Reviewed-by: Dr. David Alan Gilbert <[e-mail]>
> > >>> ---
> > >>>   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

Paolo Bonzini March 15, 2021, 11:58 a.m. UTC | #6

On 15/03/21 12:38, Dr. David Alan Gilbert wrote:
> * Kevin Wolf ([e-mail]) wrote:
>> Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
>>> Paolo Bonzini <[e-mail]> writes:
>>>
>>>> On 13/03/21 14:28, Markus Armbruster wrote:
>>>>> Kevin Wolf <[e-mail]> 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 <[e-mail]>
>>>>>> Acked-by: Peter Krempa <[e-mail]>
>>>>>> Reviewed-by: Eric Blake <[e-mail]>
>>>>>> Reviewed-by: Dr. David Alan Gilbert <[e-mail]>
>>>>>> ---
>>>>>>    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

Patch

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,