Re: [PATCH v5 22/35] monitor: implement "user creatable" interface for adding monitors

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Implement the user creatable QOM interface and define the monitor-qmp
> and monitor-hmp types in QAPI. This unlocks the ability to create them
> on the command line with -object or in HMP/QMP with object_add.
>
> For example:
>
>   $QEMU -chardev stdio,id=monchr0 -object monitor-hmp,id=mon0,chrdev=monchr0
>
> Initially the "prepare_delete" callback is hardcoded to return an error
> which means -object and object_add can be used, but object_del will fail.
> Support for deleting monitors will be introduced in subsequent commits.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Tested-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

[...]

> diff --git a/qapi/qom.json b/qapi/qom.json
> index dd45ac1087..6affb70a59 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -1187,6 +1187,45 @@
>    'data': { '*cpu-affinity': ['uint16'],
>              '*node-affinity': ['uint16'] } }
>  
> +##
> +# @MonitorProperties:
> +#
> +# Properties for all monitors
> +#
> +# @chardev: ID of the character device providing the monitor transport
> +#
> +# Since: 11.1
> +##
> +{ 'struct': 'MonitorProperties',
> +  'data': { 'chardev': 'str' }}
> +
> +##
> +# @MonitorHMPProperties:
> +#
> +# Properties for the HMP monitor
> +#
> +# @readline: whether to enable readline for line editing
> +#      (default: enabled)

The default value is 'true', and it means 'enabeld'.  We pretty much
universally have the default value in (default: ...), not the meaning.

> +#
> +# Since: 11.1
> +##
> +{ 'struct': 'MonitorHMPProperties',
> +  'base': 'MonitorProperties',
> +  'data': { '*readline': 'bool' } }
> +
> +##
> +# @MonitorQMPProperties:
> +#
> +# Properties for the QMP monitor
> +#
> +# @pretty: whether to pretty print JSON responses (default: disabled)

Likewise.

> +#
> +# Since: 11.1
> +##
> +{ 'struct': 'MonitorQMPProperties',
> +  'base': 'MonitorProperties',
> +  'data': { '*pretty': 'bool' } }
> +
>  ##
>  # @ObjectType:
>  #
> @@ -1237,6 +1276,8 @@
>      'memory-backend-ram',
>      { 'name': 'memory-backend-shm',
>        'if': 'CONFIG_POSIX' },
> +    'monitor-hmp',
> +    'monitor-qmp',
>      'pef-guest',
>      { 'name': 'pr-manager-helper',
>        'if': 'CONFIG_LINUX' },
> @@ -1315,6 +1356,8 @@
>        'memory-backend-ram':         'MemoryBackendProperties',
>        'memory-backend-shm':         { 'type': 'MemoryBackendShmProperties',
>                                        'if': 'CONFIG_POSIX' },
> +      'monitor-hmp':                {'type': 'MonitorHMPProperties' },
> +      'monitor-qmp':                {'type': 'MonitorQMPProperties' },

Use the short form when possible:

         'monitor-hmp':                'MonitorHMPProperties',
         'monitor-qmp':                'MonitorQMPProperties',

>        'pr-manager-helper':          { 'type': 'PrManagerHelperProperties',
>                                        'if': 'CONFIG_LINUX' },
>        'qtest':                      'QtestProperties',
> diff --git a/stubs/monitor-internal.c b/stubs/monitor-internal.c
> index 51db7588b9..59ccc4b35c 100644
> --- a/stubs/monitor-internal.c
> +++ b/stubs/monitor-internal.c
> @@ -10,4 +10,5 @@ int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
>  
>  void monitor_new_hmp(const char *chardev_id, bool use_readline, Error **errp)
>  {
> +    g_assert_not_reached();
>  }
> diff --git a/system/vl.c b/system/vl.c
> index fb36140aac..79ed2be449 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -1830,6 +1830,10 @@ static void object_option_add_visitor(Visitor *v)
>  {
>      ObjectOption *opt = g_new0(ObjectOption, 1);
>      visit_type_ObjectOptions(v, NULL, &opt->opts, &error_fatal);
> +    if (opt->opts->qom_type == OBJECT_TYPE_MONITOR_HMP ||
> +        opt->opts->qom_type == OBJECT_TYPE_MONITOR_QMP) {
> +        default_monitor = 0;
> +    }

Still okay, but if we pick up more, we better do it like
default_driver_disable() does for devices.

>      QTAILQ_INSERT_TAIL(&object_opts, opt, next);
>  }
>  
> @@ -1971,7 +1975,9 @@ static bool object_create_early(const char *type)
>  
>      /* Reason: property "chardev" */
>      if (g_str_equal(type, "rng-egd") ||
> -        g_str_equal(type, "qtest")) {
> +        g_str_equal(type, "qtest") ||
> +        g_str_equal(type, "monitor-hmp") ||
> +        g_str_equal(type, "monitor-qmp")) {
>          return false;
>      }

QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>