[Qemu-devel,RFC] spice-core: allow setting properties from QMP

Submitted by Kevin Pouget on June 20, 2019, 11:54 a.m.

Details

Message ID CADJ1XR3fh0cyOerSM8VQkpde6cHLb8WccP05Rwr7xWMOK59rog@mail.gmail.com
State New
Headers show
Series "spice-core: allow setting properties from QMP" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Kevin Pouget June 20, 2019, 11:54 a.m.
Hello Eric,

> A new command may be okay, however,

thanks, I've fix the typos and updated the patch to use an Enum, which
indeed makes more sense.

I've also updated "spice-query" command to provide the current value
of the "video-codec" property,
but it made me wonder if I should improve this QMP interface with a
json list, or keep the current string-based list
("enc1:codec1;enc2:codec2").

I CC the spice-devel list to get their point of view

The current behavior is:

--> { "execute": "set-spice", "arguments": { "property":
"video-codecs", "value": "spice:mjpeg;gstreamer:h264" } }
<-- {"return":{},"id":"libvirt-23"}

--> { "execute": "query-spice" }
<-- {.... "video-codecs": "spice:mjpeg;gstreamer:h264;" ....}


I put the new version of the RFC patch below

best regards,

Kevin

---

This patch allows setting spice properties from the QMP interface.

At the moment, only the 'video-codecs' property is supported.

Signed-off-by: Kevin Pouget <kpouget@redhat.com>
---
 qapi/ui.json    | 42 ++++++++++++++++++++++++++++++++++++++++--
 ui/spice-core.c | 21 +++++++++++++++++++++
 2 files changed, 61 insertions(+), 2 deletions(-)

     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
@@ -555,6 +574,8 @@ SpiceInfo *qmp_query_spice(Error **errp)
                        SPICE_QUERY_MOUSE_MODE_SERVER :
                        SPICE_QUERY_MOUSE_MODE_CLIENT;

+    info->video_codecs = spice_server_get_video_codecs(spice_server);
+
     /* for compatibility with the original command */
     info->has_channels = true;
     info->channels = qmp_query_spice_channels();

Patch hide | download patch | download mbox

diff --git a/qapi/ui.json b/qapi/ui.json
index 59e412139a..5f67096bcb 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -211,12 +211,16 @@ 
 #
 # @channels: a list of @SpiceChannel for each active spice channel
 #
+# @video-codecs: The list of encoders:codecs currently allowed for
+#                video streaming (since: ...)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'SpiceInfo',
   'data': {'enabled': 'bool', 'migrated': 'bool', '*host': 'str',
'*port': 'int',
            '*tls-port': 'int', '*auth': 'str', '*compiled-version': 'str',
-           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel']},
+           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel'],
+           'video-codecs': 'str'},
   'if': 'defined(CONFIG_SPICE)' }

 ##
@@ -257,7 +261,8 @@ 
 #                "tls": false
 #             },
 #             [ ... more channels follow ... ]
-#          ]
+#          ],
+#          "video-codecs": "spice:mjpeg;gstreamer:h264;"
 #       }
 #    }
 #
@@ -265,6 +270,39 @@ 
 { 'command': 'query-spice', 'returns': 'SpiceInfo',
   'if': 'defined(CONFIG_SPICE)' }

+##
+# @SpiceProperty:
+#
+# An enumeration of Spice properties that can be set at runtime.
+#
+# @video-codecs: the ;-separated list of video-codecs allowed for
+# spice-server video streaming.
+#
+# Since: ...
+##
+{ 'enum': 'SpiceProperty',
+  'data': [ 'video-codecs'],
+  'if': 'defined(CONFIG_SPICE)' }
+
+##
+# @set-spice:
+#
+# Set Spice properties.
+# @property: the SPICE property to modify
+# @value: the new value to affect to this property
+#
+# Since: ...
+#
+# Example:
+#
+# -> { "execute": "set-spice", "arguments": { "property": "video-codecs",
+#                                             "value": "spice:mjpeg;" } }
+# <- { "returns": {} }
+##
+{ 'command': 'set-spice',
+  'data': {'property': 'SpiceProperty', 'value': 'str'},
+  'if': 'defined(CONFIG_SPICE)' }
+
 ##
 # @SPICE_CONNECTED:
 #
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 63e8694df8..1660f49f15 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -506,6 +506,25 @@  static QemuOptsList qemu_spice_opts = {
     },
 };

+void qmp_set_spice(SpiceProperty property, const char *value, Error **errp)
+{
+    int invalid_codecs;
+
+    switch(property) {
+    case SPICE_PROPERTY_VIDEO_CODECS:
+        invalid_codecs = spice_server_set_video_codecs(spice_server, value);
+
+        if (invalid_codecs) {
+            error_setg(errp, "Found %d invalid video-codecs while
setting spice"
+                       " property 'video-codec=%s'", invalid_codecs, value);
+        }
+        break;
+    default:
+        /* only reached in case of version mismatched */
+        error_setg(errp, "Property #%d not supported.", property);
+    }
+}
+
 SpiceInfo *qmp_query_spice(Error **errp)
 {

Comments

> 
> Hello Eric,
> 
> > A new command may be okay, however,
> 
> thanks, I've fix the typos and updated the patch to use an Enum, which
> indeed makes more sense.
> 
> I've also updated "spice-query" command to provide the current value
> of the "video-codec" property,
> but it made me wonder if I should improve this QMP interface with a
> json list, or keep the current string-based list
> ("enc1:codec1;enc2:codec2").
> 
> I CC the spice-devel list to get their point of view
> 
> The current behavior is:
> 
> --> { "execute": "set-spice", "arguments": { "property":
> "video-codecs", "value": "spice:mjpeg;gstreamer:h264" } }
> <-- {"return":{},"id":"libvirt-23"}

It looks complicated from the user. Why not just

--> { "execute": "set-spice", "arguments": { "video-codecs": "spice:mjpeg;gstreamer:h264" } }

> 
> --> { "execute": "query-spice" }
> <-- {.... "video-codecs": "spice:mjpeg;gstreamer:h264;" ....}
> 
> 
> I put the new version of the RFC patch below
> 
> best regards,
> 
> Kevin
> 
> ---
> 
> This patch allows setting spice properties from the QMP interface.
> 
> At the moment, only the 'video-codecs' property is supported.
> 
> Signed-off-by: Kevin Pouget <kpouget@redhat.com>
> ---
>  qapi/ui.json    | 42 ++++++++++++++++++++++++++++++++++++++++--
>  ui/spice-core.c | 21 +++++++++++++++++++++
>  2 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 59e412139a..5f67096bcb 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -211,12 +211,16 @@
>  #
>  # @channels: a list of @SpiceChannel for each active spice channel
>  #
> +# @video-codecs: The list of encoders:codecs currently allowed for
> +#                video streaming (since: ...)
> +#
>  # Since: 0.14.0
>  ##
>  { 'struct': 'SpiceInfo',
>    'data': {'enabled': 'bool', 'migrated': 'bool', '*host': 'str',
> '*port': 'int',
>             '*tls-port': 'int', '*auth': 'str', '*compiled-version': 'str',
> -           'mouse-mode': 'SpiceQueryMouseMode', '*channels':
> ['SpiceChannel']},
> +           'mouse-mode': 'SpiceQueryMouseMode', '*channels':
> ['SpiceChannel'],
> +           'video-codecs': 'str'},
>    'if': 'defined(CONFIG_SPICE)' }
> 
>  ##
> @@ -257,7 +261,8 @@
>  #                "tls": false
>  #             },
>  #             [ ... more channels follow ... ]
> -#          ]
> +#          ],
> +#          "video-codecs": "spice:mjpeg;gstreamer:h264;"
>  #       }
>  #    }
>  #
> @@ -265,6 +270,39 @@
>  { 'command': 'query-spice', 'returns': 'SpiceInfo',
>    'if': 'defined(CONFIG_SPICE)' }
> 
> +##
> +# @SpiceProperty:
> +#
> +# An enumeration of Spice properties that can be set at runtime.
> +#
> +# @video-codecs: the ;-separated list of video-codecs allowed for
> +# spice-server video streaming.
> +#
> +# Since: ...
> +##
> +{ 'enum': 'SpiceProperty',
> +  'data': [ 'video-codecs'],
> +  'if': 'defined(CONFIG_SPICE)' }
> +
> +##
> +# @set-spice:
> +#
> +# Set Spice properties.
> +# @property: the SPICE property to modify
> +# @value: the new value to affect to this property
> +#
> +# Since: ...
> +#
> +# Example:
> +#
> +# -> { "execute": "set-spice", "arguments": { "property": "video-codecs",
> +#                                             "value": "spice:mjpeg;" } }
> +# <- { "returns": {} }
> +##
> +{ 'command': 'set-spice',
> +  'data': {'property': 'SpiceProperty', 'value': 'str'},
> +  'if': 'defined(CONFIG_SPICE)' }
> +
>  ##
>  # @SPICE_CONNECTED:
>  #
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 63e8694df8..1660f49f15 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -506,6 +506,25 @@ static QemuOptsList qemu_spice_opts = {
>      },
>  };
> 
> +void qmp_set_spice(SpiceProperty property, const char *value, Error **errp)
> +{
> +    int invalid_codecs;
> +
> +    switch(property) {
> +    case SPICE_PROPERTY_VIDEO_CODECS:
> +        invalid_codecs = spice_server_set_video_codecs(spice_server, value);
> +
> +        if (invalid_codecs) {
> +            error_setg(errp, "Found %d invalid video-codecs while
> setting spice"
> +                       " property 'video-codec=%s'", invalid_codecs, value);
> +        }
> +        break;
> +    default:
> +        /* only reached in case of version mismatched */
> +        error_setg(errp, "Property #%d not supported.", property);
> +    }
> +}
> +
>  SpiceInfo *qmp_query_spice(Error **errp)
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
> @@ -555,6 +574,8 @@ SpiceInfo *qmp_query_spice(Error **errp)
>                         SPICE_QUERY_MOUSE_MODE_SERVER :
>                         SPICE_QUERY_MOUSE_MODE_CLIENT;
> 
> +    info->video_codecs = spice_server_get_video_codecs(spice_server);
> +
>      /* for compatibility with the original command */
>      info->has_channels = true;
>      info->channels = qmp_query_spice_channels();
> --
> 2.21.0
> 
>