| Message ID | 20190619123042.4822-1-kpouget@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] spice-core: allow setting properties from QMP | expand |
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
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 > >
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();
> > 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 > >
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();
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);
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(+) -- 2.21.0