* [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help"
@ 2021-01-20 14:42 Paolo Bonzini
2021-01-20 14:42 ` [PATCH 1/3] hmp: remove "change vnc TARGET" command Paolo Bonzini
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-01-20 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, kraxel
The real driver for these patches is to send all QemuOpts user input
to qemu_opts_parse_noisily, for consistency in the command line
parsing code and to effectively outlaw "help" and "?" QemuOpts
suboptions. vnc_parse is the only function that is still using
qemu_opts_parse.
In order to remove the non-command-line callers of vnc_parse,
I am removing the deprecated QMP change command but also its HMP
veneer "change vnc TARGET", whose usecase is somewhat unclear to
me. "change vnc password" is still supported.
Finally, by switching to qemu_opts_parse_noisily, it is easy to
print a help message on "-vnc help".
Paolo Bonzini (3):
hmp: remove "change vnc TARGET" command
qmp: remove deprecated "change" command
vnc: support "-vnc help"
docs/system/deprecated.rst | 5 ----
docs/system/removed-features.rst | 11 +++++++
hmp-commands.hx | 6 ----
include/ui/console.h | 2 +-
monitor/hmp-cmds.c | 7 +++--
monitor/qmp-cmds.c | 51 --------------------------------
qapi/misc.json | 49 ------------------------------
softmmu/vl.c | 6 ++--
ui/vnc-stubs.c | 7 ++---
ui/vnc.c | 8 ++---
10 files changed, 27 insertions(+), 125 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] hmp: remove "change vnc TARGET" command
2021-01-20 14:42 [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help" Paolo Bonzini
@ 2021-01-20 14:42 ` Paolo Bonzini
2021-01-20 15:24 ` Eric Blake
2021-01-20 14:42 ` [PATCH 2/3] qmp: remove deprecated "change" command Paolo Bonzini
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2021-01-20 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, kraxel
The HMP command \"change vnc TARGET\" is messy:
- it takes an ugly shortcut to determine if the option has an "id",
with incorrect results if "id=" is not preceded by an unescaped
comma.
- it deletes the existing QemuOpts and does not try to rollback
if the parsing fails (which is not causing problems, but only due to
how VNC options are parsed)
- because it uses the same parsing function as "-vnc", it forces
the latter to not support "-vnc help".
On top of this, it uses a deprecated QMP command, thus getting in
the way of removing the QMP command. Since the usecase for the
command is not clear, just remove it and send "change vnc password"
directly to the QMP "change-vnc-password" command.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/system/removed-features.rst | 6 ++++++
hmp-commands.hx | 6 ------
monitor/hmp-cmds.c | 7 +++++--
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 430fc33ca1..5b0ff6ab1f 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -68,6 +68,12 @@ The ``[hub_id name]`` parameter tuple of the 'hostfwd_add' and
Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``. See
documentation of ``query-hotpluggable-cpus`` for additional details.
+``change vnc TARGET`` (removed in 6.0)
+''''''''''''''''''''''''''''''''''''''
+
+No replacement. The ``change vnc password`` and ``change DEVICE MEDIUM``
+commands are not affected.
+
Guest Emulator ISAs
-------------------
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 73e0832ea1..d4001f9c5d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -231,12 +231,6 @@ SRST
read-write
Makes the device writable.
- ``change vnc`` *display*,\ *options*
- Change the configuration of the VNC server. The valid syntax for *display*
- and *options* are described at :ref:`sec_005finvocation`. eg::
-
- (qemu) change vnc localhost:1
-
``change vnc password`` [*password*]
Change the password associated with the VNC server. If the new password
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index fd4d77e246..499647a578 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1521,13 +1521,16 @@ void hmp_change(Monitor *mon, const QDict *qdict)
}
if (strcmp(target, "passwd") == 0 ||
strcmp(target, "password") == 0) {
- if (!arg) {
+ if (arg) {
MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
return;
+ } else {
+ qmp_change_vnc_password(arg, &err);
}
+ } else {
+ monitor_printf(mon, "Expected 'password' after 'vnc'\n");
}
- qmp_change("vnc", target, !!arg, arg, &err);
} else
#endif
{
--
2.29.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] qmp: remove deprecated "change" command
2021-01-20 14:42 [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help" Paolo Bonzini
2021-01-20 14:42 ` [PATCH 1/3] hmp: remove "change vnc TARGET" command Paolo Bonzini
@ 2021-01-20 14:42 ` Paolo Bonzini
2021-01-20 15:21 ` Philippe Mathieu-Daudé
2021-01-20 15:31 ` Eric Blake
2021-01-20 14:42 ` [PATCH 3/3] vnc: support "-vnc help" Paolo Bonzini
2021-01-21 10:38 ` [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, " Gerd Hoffmann
3 siblings, 2 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-01-20 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, kraxel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/system/deprecated.rst | 5 ----
docs/system/removed-features.rst | 5 ++++
monitor/qmp-cmds.c | 51 --------------------------------
qapi/misc.json | 49 ------------------------------
4 files changed, 5 insertions(+), 105 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index e20bfcb17a..651182b2df 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -131,11 +131,6 @@ devices. It is possible to use drives the board doesn't pick up with
QEMU Machine Protocol (QMP) commands
------------------------------------
-``change`` (since 2.5.0)
-''''''''''''''''''''''''
-
-Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
-
``blockdev-open-tray``, ``blockdev-close-tray`` argument ``device`` (since 2.8.0)
'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index 5b0ff6ab1f..c48266724f 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -53,6 +53,11 @@ are automatically loaded from qcow2 images.
Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``. See
documentation of ``query-hotpluggable-cpus`` for additional details.
+``change`` (removed in 6.0)
+'''''''''''''''''''''''''''
+
+Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
+
Human Monitor Protocol (HMP) commands
-------------------------------------
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 34f7e75b7b..990936136c 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -251,58 +251,7 @@ void qmp_change_vnc_password(const char *password, Error **errp)
error_setg(errp, "Could not set password");
}
}
-
-static void qmp_change_vnc_listen(const char *target, Error **errp)
-{
- QemuOptsList *olist = qemu_find_opts("vnc");
- QemuOpts *opts;
-
- if (strstr(target, "id=")) {
- error_setg(errp, "id not supported");
- return;
- }
-
- opts = qemu_opts_find(olist, "default");
- if (opts) {
- qemu_opts_del(opts);
- }
- opts = vnc_parse(target, errp);
- if (!opts) {
- return;
- }
-
- vnc_display_open("default", errp);
-}
-
-static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
- Error **errp)
-{
- if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
- if (!has_arg) {
- error_setg(errp, QERR_MISSING_PARAMETER, "password");
- } else {
- qmp_change_vnc_password(arg, errp);
- }
- } else {
- qmp_change_vnc_listen(target, errp);
- }
-}
-#endif /* !CONFIG_VNC */
-
-void qmp_change(const char *device, const char *target,
- bool has_arg, const char *arg, Error **errp)
-{
- if (strcmp(device, "vnc") == 0) {
-#ifdef CONFIG_VNC
- qmp_change_vnc(target, has_arg, arg, errp);
-#else
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
#endif
- } else {
- qmp_blockdev_change_medium(true, device, false, NULL, target,
- has_arg, arg, false, 0, errp);
- }
-}
void qmp_add_client(const char *protocol, const char *fdname,
bool has_skipauth, bool skipauth, bool has_tls, bool tls,
diff --git a/qapi/misc.json b/qapi/misc.json
index 27ccd7385f..156f98203e 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -238,55 +238,6 @@
'returns': 'str',
'features': [ 'savevm-monitor-nodes' ] }
-##
-# @change:
-#
-# This command is multiple commands multiplexed together.
-#
-# @device: This is normally the name of a block device but it may also be 'vnc'.
-# when it's 'vnc', then sub command depends on @target
-#
-# @target: If @device is a block device, then this is the new filename.
-# If @device is 'vnc', then if the value 'password' selects the vnc
-# change password command. Otherwise, this specifies a new server URI
-# address to listen to for VNC connections.
-#
-# @arg: If @device is a block device, then this is an optional format to open
-# the device with.
-# If @device is 'vnc' and @target is 'password', this is the new VNC
-# password to set. See change-vnc-password for additional notes.
-#
-# Features:
-# @deprecated: This command is deprecated. For changing block
-# devices, use 'blockdev-change-medium' instead; for changing VNC
-# parameters, use 'change-vnc-password' instead.
-#
-# Returns: - Nothing on success.
-# - If @device is not a valid block device, DeviceNotFound
-#
-# Since: 0.14
-#
-# Example:
-#
-# 1. Change a removable medium
-#
-# -> { "execute": "change",
-# "arguments": { "device": "ide1-cd0",
-# "target": "/srv/images/Fedora-12-x86_64-DVD.iso" } }
-# <- { "return": {} }
-#
-# 2. Change VNC password
-#
-# -> { "execute": "change",
-# "arguments": { "device": "vnc", "target": "password",
-# "arg": "foobar1" } }
-# <- { "return": {} }
-#
-##
-{ 'command': 'change',
- 'data': {'device': 'str', 'target': 'str', '*arg': 'str'},
- 'features': [ 'deprecated' ] }
-
##
# @getfd:
#
--
2.29.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] vnc: support "-vnc help"
2021-01-20 14:42 [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help" Paolo Bonzini
2021-01-20 14:42 ` [PATCH 1/3] hmp: remove "change vnc TARGET" command Paolo Bonzini
2021-01-20 14:42 ` [PATCH 2/3] qmp: remove deprecated "change" command Paolo Bonzini
@ 2021-01-20 14:42 ` Paolo Bonzini
2021-01-20 15:44 ` Eric Blake
2021-01-21 10:38 ` [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, " Gerd Hoffmann
3 siblings, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2021-01-20 14:42 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, kraxel
Use qemu_opts_parse_noisily now that HMP does not call
vnc_parse anymore.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/ui/console.h | 2 +-
softmmu/vl.c | 6 +++---
ui/vnc-stubs.c | 7 +++----
ui/vnc.c | 8 ++++----
4 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index 5dd21976a3..7a3fc11abf 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -439,7 +439,7 @@ void vnc_display_open(const char *id, Error **errp);
void vnc_display_add_client(const char *id, int csock, bool skipauth);
int vnc_display_password(const char *id, const char *password);
int vnc_display_pw_expire(const char *id, time_t expires);
-QemuOpts *vnc_parse(const char *str, Error **errp);
+void vnc_parse(const char *str);
int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
/* input.c */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 59304261cf..a8876b8965 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1113,7 +1113,7 @@ static void parse_display(const char *p)
* display access.
*/
if (*opts == '=') {
- vnc_parse(opts + 1, &error_fatal);
+ vnc_parse(opts + 1);
} else {
error_report("VNC requires a display argument vnc=<display>");
exit(1);
@@ -1402,7 +1402,7 @@ static void qemu_create_default_devices(void)
if (!qemu_display_find_default(&dpy)) {
dpy.type = DISPLAY_TYPE_NONE;
#if defined(CONFIG_VNC)
- vnc_parse("localhost:0,to=99,id=default", &error_abort);
+ vnc_parse("localhost:0,to=99,id=default");
#endif
}
}
@@ -3186,7 +3186,7 @@ void qemu_init(int argc, char **argv, char **envp)
}
break;
case QEMU_OPTION_vnc:
- vnc_parse(optarg, &error_fatal);
+ vnc_parse(optarg);
break;
case QEMU_OPTION_no_acpi:
olist = qemu_find_opts("machine");
diff --git a/ui/vnc-stubs.c b/ui/vnc-stubs.c
index c6b737dcec..0fd4e0846a 100644
--- a/ui/vnc-stubs.c
+++ b/ui/vnc-stubs.c
@@ -10,13 +10,12 @@ int vnc_display_pw_expire(const char *id, time_t expires)
{
return -ENODEV;
};
-QemuOpts *vnc_parse(const char *str, Error **errp)
+void vnc_parse(const char *str)
{
if (strcmp(str, "none") == 0) {
- return NULL;
+ return;
}
- error_setg(errp, "VNC support is disabled");
- return NULL;
+ error_setg(&error_fatal, "VNC support is disabled");
}
int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
{
diff --git a/ui/vnc.c b/ui/vnc.c
index d429bfee5a..66f7c1b936 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -50,6 +50,7 @@
#include "crypto/random.h"
#include "qom/object_interfaces.h"
#include "qemu/cutils.h"
+#include "qemu/help_option.h"
#include "io/dns-resolver.h"
#define VNC_REFRESH_INTERVAL_BASE GUI_REFRESH_INTERVAL_DEFAULT
@@ -4211,14 +4212,14 @@ static void vnc_auto_assign_id(QemuOptsList *olist, QemuOpts *opts)
qemu_opts_set_id(opts, id);
}
-QemuOpts *vnc_parse(const char *str, Error **errp)
+void vnc_parse(const char *str)
{
QemuOptsList *olist = qemu_find_opts("vnc");
- QemuOpts *opts = qemu_opts_parse(olist, str, true, errp);
+ QemuOpts *opts = qemu_opts_parse_noisily(olist, str, !is_help_option(str));
const char *id;
if (!opts) {
- return NULL;
+ exit(1);
}
id = qemu_opts_id(opts);
@@ -4226,7 +4227,6 @@ QemuOpts *vnc_parse(const char *str, Error **errp)
/* auto-assign id if not present */
vnc_auto_assign_id(olist, opts);
}
- return opts;
}
int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
--
2.29.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] qmp: remove deprecated "change" command
2021-01-20 14:42 ` [PATCH 2/3] qmp: remove deprecated "change" command Paolo Bonzini
@ 2021-01-20 15:21 ` Philippe Mathieu-Daudé
2021-01-20 15:31 ` Eric Blake
1 sibling, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-20 15:21 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: armbru, kraxel
On 1/20/21 3:42 PM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> docs/system/deprecated.rst | 5 ----
> docs/system/removed-features.rst | 5 ++++
> monitor/qmp-cmds.c | 51 --------------------------------
> qapi/misc.json | 49 ------------------------------
> 4 files changed, 5 insertions(+), 105 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] hmp: remove "change vnc TARGET" command
2021-01-20 14:42 ` [PATCH 1/3] hmp: remove "change vnc TARGET" command Paolo Bonzini
@ 2021-01-20 15:24 ` Eric Blake
0 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2021-01-20 15:24 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: armbru, kraxel
On 1/20/21 8:42 AM, Paolo Bonzini wrote:
> The HMP command \"change vnc TARGET\" is messy:
>
> - it takes an ugly shortcut to determine if the option has an "id",
> with incorrect results if "id=" is not preceded by an unescaped
> comma.
>
> - it deletes the existing QemuOpts and does not try to rollback
> if the parsing fails (which is not causing problems, but only due to
> how VNC options are parsed)
>
> - because it uses the same parsing function as "-vnc", it forces
> the latter to not support "-vnc help".
>
> On top of this, it uses a deprecated QMP command, thus getting in
> the way of removing the QMP command. Since the usecase for the
> command is not clear, just remove it and send "change vnc password"
> directly to the QMP "change-vnc-password" command.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> docs/system/removed-features.rst | 6 ++++++
> hmp-commands.hx | 6 ------
> monitor/hmp-cmds.c | 7 +++++--
> 3 files changed, 11 insertions(+), 8 deletions(-)
HMP is not promised to be stable, so no deprecation period required.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] qmp: remove deprecated "change" command
2021-01-20 14:42 ` [PATCH 2/3] qmp: remove deprecated "change" command Paolo Bonzini
2021-01-20 15:21 ` Philippe Mathieu-Daudé
@ 2021-01-20 15:31 ` Eric Blake
1 sibling, 0 replies; 12+ messages in thread
From: Eric Blake @ 2021-01-20 15:31 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: armbru, kraxel
On 1/20/21 8:42 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> docs/system/deprecated.rst | 5 ----
> docs/system/removed-features.rst | 5 ++++
> monitor/qmp-cmds.c | 51 --------------------------------
> qapi/misc.json | 49 ------------------------------
> 4 files changed, 5 insertions(+), 105 deletions(-)
>
> -``change`` (since 2.5.0)
> -''''''''''''''''''''''''
> -
> -Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
We didn't document a fallback for changing the vnc listener...
> +++ b/docs/system/removed-features.rst
> @@ -53,6 +53,11 @@ are automatically loaded from qcow2 images.
> Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``. See
> documentation of ``query-hotpluggable-cpus`` for additional details.
>
> +``change`` (removed in 6.0)
> +'''''''''''''''''''''''''''
> +
> +Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
...and still don't,...
> -static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> - Error **errp)
> -{
> - if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) {
> - if (!has_arg) {
> - error_setg(errp, QERR_MISSING_PARAMETER, "password");
> - } else {
> - qmp_change_vnc_password(arg, errp);
> - }
> - } else {
> - qmp_change_vnc_listen(target, errp);
...and now we're entirely removing that QMP ability, without
deprecation. Is that intended?
> +++ b/qapi/misc.json
> @@ -238,55 +238,6 @@
> 'returns': 'str',
> 'features': [ 'savevm-monitor-nodes' ] }
>
> -##
> -# @change:
> -#
> -# This command is multiple commands multiplexed together.
> -#
> -# @device: This is normally the name of a block device but it may also be 'vnc'.
> -# when it's 'vnc', then sub command depends on @target
> -#
> -# @target: If @device is a block device, then this is the new filename.
> -# If @device is 'vnc', then if the value 'password' selects the vnc
> -# change password command. Otherwise, this specifies a new server URI
> -# address to listen to for VNC connections.
That is, I'm wondering if we should be adding a change-vnc-listener QMP
command, and extending the deprecation for 2 more releases to allow this
final use of 'change vnc' to still work. Or are we really at the point
where we have no known users?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] vnc: support "-vnc help"
2021-01-20 14:42 ` [PATCH 3/3] vnc: support "-vnc help" Paolo Bonzini
@ 2021-01-20 15:44 ` Eric Blake
0 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2021-01-20 15:44 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: armbru, kraxel
On 1/20/21 8:42 AM, Paolo Bonzini wrote:
> Use qemu_opts_parse_noisily now that HMP does not call
> vnc_parse anymore.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/ui/console.h | 2 +-
> softmmu/vl.c | 6 +++---
> ui/vnc-stubs.c | 7 +++----
> ui/vnc.c | 8 ++++----
> 4 files changed, 11 insertions(+), 12 deletions(-)
Unless something about my comments on 2/3 causes us to change,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help"
2021-01-20 14:42 [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help" Paolo Bonzini
` (2 preceding siblings ...)
2021-01-20 14:42 ` [PATCH 3/3] vnc: support "-vnc help" Paolo Bonzini
@ 2021-01-21 10:38 ` Gerd Hoffmann
2021-01-21 10:52 ` Daniel P. Berrangé
3 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2021-01-21 10:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru
On Wed, Jan 20, 2021 at 03:42:32PM +0100, Paolo Bonzini wrote:
> The real driver for these patches is to send all QemuOpts user input
> to qemu_opts_parse_noisily, for consistency in the command line
> parsing code and to effectively outlaw "help" and "?" QemuOpts
> suboptions. vnc_parse is the only function that is still using
> qemu_opts_parse.
Should we maybe move vnc to qapi cmd line parsing instead?
> In order to remove the non-command-line callers of vnc_parse,
> I am removing the deprecated QMP change command but also its HMP
> veneer "change vnc TARGET", whose usecase is somewhat unclear to
> me.
Hmm. It's been a few years ...
IIRC back when this was added the main use case was having a way to
enable/disable the vnc server. Not sure this is still needed/useful.
These days you can effectively disable vnc access by expiring the
password (or not setting one in the first place) without re-configuring
the vnc server. Also the race where qemu allowed passwordless connects
between start and password being set via monitor is long gone.
So, all in all I feel a bit uncomfortable dropping this without the
usual deprecation period. No strong objections though.
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help"
2021-01-21 10:38 ` [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, " Gerd Hoffmann
@ 2021-01-21 10:52 ` Daniel P. Berrangé
2021-01-21 11:13 ` Gerd Hoffmann
0 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2021-01-21 10:52 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Paolo Bonzini, qemu-devel, armbru
On Thu, Jan 21, 2021 at 11:38:31AM +0100, Gerd Hoffmann wrote:
> On Wed, Jan 20, 2021 at 03:42:32PM +0100, Paolo Bonzini wrote:
> > The real driver for these patches is to send all QemuOpts user input
> > to qemu_opts_parse_noisily, for consistency in the command line
> > parsing code and to effectively outlaw "help" and "?" QemuOpts
> > suboptions. vnc_parse is the only function that is still using
> > qemu_opts_parse.
>
> Should we maybe move vnc to qapi cmd line parsing instead?
>
> > In order to remove the non-command-line callers of vnc_parse,
> > I am removing the deprecated QMP change command but also its HMP
> > veneer "change vnc TARGET", whose usecase is somewhat unclear to
> > me.
>
> Hmm. It's been a few years ...
>
> IIRC back when this was added the main use case was having a way to
> enable/disable the vnc server. Not sure this is still needed/useful.
> These days you can effectively disable vnc access by expiring the
> password (or not setting one in the first place) without re-configuring
> the vnc server. Also the race where qemu allowed passwordless connects
> between start and password being set via monitor is long gone.
Yep, it was my patch back here:
https://lists.gnu.org/archive/html/qemu-devel/2007-08/msg00151.html
The original justification or design was not especially compelling
and somewhat hackish in retrospect.
These days we really ought to change VNC so that it integrates with
"-object secret" for getting its password.
Being able to live add/remove display backends is somewhat
interesting, but if we want that it should be done generically
and use qapi modelling, and covering at least SPICE and VNC.
> So, all in all I feel a bit uncomfortable dropping this without the
> usual deprecation period. No strong objections though.
Well we did deprecate the "change" command in general in 2.5.0.
https://qemu.readthedocs.io/en/latest/system/deprecated.html#change-since-2-5-0
We gave illustrations for replacement for vnc password and CD
media change, but no replacement was provided for changing
VNC server config. That's ok though, as there's no requirement
that we provide a replacement when deprecating stuff. It would
have been nice if we explicitly mentioned we were dropping the
vnc target change funcitonality, but we have none the less
followed deprecation process for the 'change' command and so
can remove it now if desired.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help"
2021-01-21 10:52 ` Daniel P. Berrangé
@ 2021-01-21 11:13 ` Gerd Hoffmann
2021-01-21 12:02 ` Paolo Bonzini
0 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2021-01-21 11:13 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: Paolo Bonzini, qemu-devel, armbru
Hi,
> > So, all in all I feel a bit uncomfortable dropping this without the
> > usual deprecation period. No strong objections though.
>
> Well we did deprecate the "change" command in general in 2.5.0.
>
> https://qemu.readthedocs.io/en/latest/system/deprecated.html#change-since-2-5-0
>
> We gave illustrations for replacement for vnc password and CD
> media change, but no replacement was provided for changing
> VNC server config. That's ok though, as there's no requirement
> that we provide a replacement when deprecating stuff.
Fair enough.
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
(assuming you want queue that with other qemu-opts changes,
but I can also take this through ui queue if you want).
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help"
2021-01-21 11:13 ` Gerd Hoffmann
@ 2021-01-21 12:02 ` Paolo Bonzini
0 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-01-21 12:02 UTC (permalink / raw)
To: Gerd Hoffmann, Daniel P. Berrangé; +Cc: qemu-devel, armbru
On 21/01/21 12:13, Gerd Hoffmann wrote:
> Hi,
>
>>> So, all in all I feel a bit uncomfortable dropping this without the
>>> usual deprecation period. No strong objections though.
>>
>> Well we did deprecate the "change" command in general in 2.5.0.
>>
>> https://qemu.readthedocs.io/en/latest/system/deprecated.html#change-since-2-5-0
>>
>> We gave illustrations for replacement for vnc password and CD
>> media change, but no replacement was provided for changing
>> VNC server config. That's ok though, as there's no requirement
>> that we provide a replacement when deprecating stuff.
>
> Fair enough.
>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>
> (assuming you want queue that with other qemu-opts changes,
> but I can also take this through ui queue if you want).
I'll queue it myself, thanks!
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-01-21 12:04 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-20 14:42 [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, support "-vnc help" Paolo Bonzini
2021-01-20 14:42 ` [PATCH 1/3] hmp: remove "change vnc TARGET" command Paolo Bonzini
2021-01-20 15:24 ` Eric Blake
2021-01-20 14:42 ` [PATCH 2/3] qmp: remove deprecated "change" command Paolo Bonzini
2021-01-20 15:21 ` Philippe Mathieu-Daudé
2021-01-20 15:31 ` Eric Blake
2021-01-20 14:42 ` [PATCH 3/3] vnc: support "-vnc help" Paolo Bonzini
2021-01-20 15:44 ` Eric Blake
2021-01-21 10:38 ` [PATCH 0/3] vnc: remove "change vnc TARGET" and QMP change command, " Gerd Hoffmann
2021-01-21 10:52 ` Daniel P. Berrangé
2021-01-21 11:13 ` Gerd Hoffmann
2021-01-21 12:02 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).