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

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

[Kevin Pouget]

Hello,

we're investigating the possibility to set some spice properties at
runtime, through the QMP interface, but we're not sure what's the best
way to proceed.
I've prepared the patch below, that adds a new QMP
command, but is there another way like with a QOM object, that could
reuse an existing command? I searched but couldn't find an easy/not
hacky way to create such objects ...

thanks,

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    | 19 +++++++++++++++++++
 ui/spice-core.c | 13 +++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/qapi/ui.json b/qapi/ui.json
index 59e412139a..5483a9c003 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -265,6 +265,25 @@
 { 'command': 'query-spice', 'returns': 'SpiceInfo',
   '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;gst:mjpeg;" } }
+# <- { "returns": {} }
+##
+{ 'command': 'set-spice',
+  'data': {'property': 'str', 'value': 'str'},
+  'if': 'defined(CONFIG_SPICE)' }
+
 ##
 # @SPICE_CONNECTED:
 #
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 2ffc3335f0..5408b16684 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -503,6 +503,19 @@ static QemuOptsList qemu_spice_opts = {
     },
 };

+void qmp_set_spice(const char *property, const char *value, Error **errp) {
+    if (strcmp(property, "video-codecs") == 0) {
+        int invalid_codecs = spice_server_set_video_codecs(spice_server, value);
+
+        if (invalid_codecs) {
+            error_setg(errp, "Found %d invalic codecs while setting "
+                       "the property %s=%s\n", invalid_codecs, property, value);
+        }
+    } else {
+        error_setg(errp, "Setting an unknown spice property (%s=%s)\n", property, value);
+    }
+}
+
 SpiceInfo *qmp_query_spice(Error **errp)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
--
2.21.0

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190619123042.4822-1-kpouget@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [RFC] spice-core: allow setting properties from QMP
Type: series
Message-id: 20190619123042.4822-1-kpouget@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190619123042.4822-1-kpouget@redhat.com -> patchew/20190619123042.4822-1-kpouget@redhat.com
Switched to a new branch 'test'
51809fd99d spice-core: allow setting properties from QMP

=== OUTPUT BEGIN ===
ERROR: open brace '{' following function declarations go on the next line
#61: FILE: ui/spice-core.c:506:
+void qmp_set_spice(const char *property, const char *value, Error **errp) {

ERROR: Error messages should not contain newlines
#67: FILE: ui/spice-core.c:512:
+                       "the property %s=%s\n", invalid_codecs, property, value);

WARNING: line over 80 characters
#70: FILE: ui/spice-core.c:515:
+        error_setg(errp, "Setting an unknown spice property (%s=%s)\n", property, value);

ERROR: Error messages should not contain newlines
#70: FILE: ui/spice-core.c:515:
+        error_setg(errp, "Setting an unknown spice property (%s=%s)\n", property, value);

ERROR: Missing Signed-off-by: line(s)

total: 4 errors, 1 warnings, 44 lines checked

Commit 51809fd99d76 (spice-core: allow setting properties from QMP) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190619123042.4822-1-kpouget@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2477 bytes --]

On 6/19/19 7:30 AM, Kevin Pouget wrote:
> Hello,
> 
> we're investigating the possibility to set some spice properties at
> runtime, through the QMP interface, but we're not sure what's the best
> way to proceed.
> I've prepared the patch below, that adds a new QMP
> command, but is there another way like with a QOM object, that could
> reuse an existing command? I searched but couldn't find an easy/not
> hacky way to create such objects ...
> 

A new command may be okay, however,


> +##
> +# @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;gst:mjpeg;" } }
> +# <- { "returns": {} }
> +##
> +{ 'command': 'set-spice',
> +  'data': {'property': 'str', 'value': 'str'},
> +  'if': 'defined(CONFIG_SPICE)' }

letting 'property' be an open-coded string feels wrong. If you are only
going to accept a specific (limited) set of property names, then
'property' should be typed as an enum:

{ 'enum': 'SpiceProperties', 'data': [ 'video-codecs' ] }

{ 'command': 'set-spice', 'data': { 'property': 'SpiceProperties',
'value': 'str' } }

> +
>  ##
>  # @SPICE_CONNECTED:
>  #
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 2ffc3335f0..5408b16684 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -503,6 +503,19 @@ static QemuOptsList qemu_spice_opts = {
>      },
>  };
> 
> +void qmp_set_spice(const char *property, const char *value, Error **errp) {
> +    if (strcmp(property, "video-codecs") == 0) {
> +        int invalid_codecs = spice_server_set_video_codecs(spice_server, value);
> +
> +        if (invalid_codecs) {
> +            error_setg(errp, "Found %d invalic codecs while setting "

invalid

> +                       "the property %s=%s\n", invalid_codecs, property, value);
> +        }
> +    } else {
> +        error_setg(errp, "Setting an unknown spice property (%s=%s)\n", property, value);
> +    }
> +}
> +
>  SpiceInfo *qmp_query_spice(Error **errp)
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
> --
> 2.21.0
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Kevin Pouget]

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(-)

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

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

[Frediano Ziglio]

> 
> 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
> 
> 

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

[Kevin Pouget]

On Fri, Jun 21, 2019 at 9:16 AM Frediano Ziglio <fziglio@redhat.com> wrote:
>
> >
> > 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" } }

it makes sense indeed, I've updated the code:

# -> { "execute": "set-spice", "arguments": { "video-codecs": "spice:mjpeg;" }
# <- { "returns": {} }

+{ 'command': 'set-spice',
+  'data': {'*video-codecs': 'str'},
+  'if': 'defined(CONFIG_SPICE)' }


---
 qapi/ui.json    | 27 +++++++++++++++++++++++++--
 ui/spice-core.c | 17 +++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 59e412139a..cdbe04bda0 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,24 @@
 { 'command': 'query-spice', 'returns': 'SpiceInfo',
   'if': 'defined(CONFIG_SPICE)' }

+##
+# @set-spice:
+#
+# Set Spice properties.
+# @video-codecs: the ;-separated list of video-codecs allowed for
+#                spice-server video streaming.
+#
+# Since: ...
+#
+# Example:
+#
+# -> { "execute": "set-spice", "arguments": { "video-codecs": "spice:mjpeg;" }
+# <- { "returns": {} }
+##
+{ 'command': 'set-spice',
+  'data': {'*video-codecs': 'str'},
+  'if': 'defined(CONFIG_SPICE)' }
+
 ##
 # @SPICE_CONNECTED:
 #
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 63e8694df8..a4b265b663 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -506,6 +506,21 @@ static QemuOptsList qemu_spice_opts = {
     },
 };

+void qmp_set_spice(bool has_video_codecs, const char *video_codecs,
+                   Error **errp)
+{
+    if (has_video_codecs) {
+        int invalid_codecs = spice_server_set_video_codecs(spice_server,
+                                                           video_codecs);
+
+        if (invalid_codecs) {
+            error_setg(errp, "Found %d invalid video-codecs while setting"
+                       " spice property 'video-codec=%s'", invalid_codecs,
+                       video_codecs);
+        }
+    }
+}
+
 SpiceInfo *qmp_query_spice(Error **errp)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
@@ -555,6 +570,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