From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E96A8C4338F for ; Wed, 25 Aug 2021 11:36:09 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 766DD61165 for ; Wed, 25 Aug 2021 11:36:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 766DD61165 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=proxmox.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:36578 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mIrCS-0002tn-Bq for qemu-devel@archiver.kernel.org; Wed, 25 Aug 2021 07:36:08 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:58402) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mIr2v-0000xn-4D for qemu-devel@nongnu.org; Wed, 25 Aug 2021 07:26:17 -0400 Received: from proxmox-new.maurer-it.com ([94.136.29.106]:9519) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mIr2s-00061O-H5 for qemu-devel@nongnu.org; Wed, 25 Aug 2021 07:26:16 -0400 Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 22F2C4389F; Wed, 25 Aug 2021 13:26:11 +0200 (CEST) Subject: Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= References: <20210825093701.668122-1-s.reiter@proxmox.com> <20210825093701.668122-3-s.reiter@proxmox.com> From: Stefan Reiter Message-ID: <4400f41b-4ecc-9340-a20d-8acdaae033be@proxmox.com> Date: Wed, 25 Aug 2021 13:26:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Received-SPF: pass client-ip=94.136.29.106; envelope-from=s.reiter@proxmox.com; helo=proxmox-new.maurer-it.com X-Spam_score_int: -40 X-Spam_score: -4.1 X-Spam_bar: ---- X-Spam_report: (-4.1 / 5.0 requ) BAYES_00=-1.9, NICE_REPLY_A=-2.24, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Wolfgang Bumiller , QEMU , "Dr. David Alan Gilbert" , Markus Armbruster , Gerd Hoffmann , Paolo Bonzini , Eric Blake , Thomas Lamprecht Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 8/25/21 12:59 PM, Marc-André Lureau wrote: > Hi > > On Wed, Aug 25, 2021 at 1:39 PM Stefan Reiter wrote: > >> It is possible to specify more than one VNC server on the command line, >> either with an explicit ID or the auto-generated ones à la "default", >> "vnc2", "vnc3", ... >> >> It is not possible to change the password on one of these extra VNC >> displays though. Fix this by adding a "display" parameter to the >> 'set_password' and 'expire_password' QMP and HMP commands. >> >> For HMP, this is a bit trickier, since at least 'set_password' already >> has the 'connected' parameter following the mandatory 'password' one, so >> we need to prefix the display ID with "id=" to allow correct parsing. >> > > It's not something done with other commands afaik, feels a bit awkward (the > "connected = display"...). > Indeed, if there is a better way I'd love to use it. One idea I had was making the parameter 'connected' OR 'display', since the former only supports 'keep' for VNC anyway - but that introduces a weird double-meaning again. > Is it really necessary to add support for HMP? > For us it would be, as we provide an easy HMP interface to our users, but not a QMP one, so it ended up being a bit of a regression with 6.0. >> With this prefix, no existing command or workflow should be affected. >> >> While rewriting the descriptions, also remove the line "Use zero to make >> the password stay valid forever." from 'set_password', I believe this was >> intended for 'expire_password', but would even be wrong there. >> >> Signed-off-by: Stefan Reiter >> --- >> hmp-commands.hx | 28 +++++++++++++++------------- >> monitor/hmp-cmds.c | 20 ++++++++++++++++++-- >> monitor/qmp-cmds.c | 9 +++++---- >> qapi/ui.json | 12 ++++++++++-- >> 4 files changed, 48 insertions(+), 21 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index e01ca13ca8..0b5abcfb8a 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -1541,34 +1541,36 @@ ERST >> >> { >> .name = "set_password", >> - .args_type = "protocol:s,password:s,connected:s?", >> - .params = "protocol password action-if-connected", >> + .args_type = "protocol:s,password:s,display:s?,connected:s?", >> + .params = "protocol password [id=display] >> [action-if-connected]", >> .help = "set spice/vnc password", >> .cmd = hmp_set_password, >> }, >> >> SRST >> -``set_password [ vnc | spice ] password [ action-if-connected ]`` >> - Change spice/vnc password. Use zero to make the password stay valid >> - forever. *action-if-connected* specifies what should happen in >> +``set_password [ vnc | spice ] password [ id=display ] [ >> action-if-connected ]`` >> + Change spice/vnc password. *display* (must be prefixed with >> + 'id=') can be used with 'vnc' to specify which display to set the >> + password on. *action-if-connected* specifies what should happen in >> case a connection is established: *fail* makes the password change >> - fail. *disconnect* changes the password and disconnects the >> - client. *keep* changes the password and keeps the connection up. >> - *keep* is the default. >> + fail. *disconnect* changes the password and disconnects the client. >> + *keep* changes the password and keeps the connection up. *keep* is >> + the default. >> ERST >> >> { >> .name = "expire_password", >> - .args_type = "protocol:s,time:s", >> - .params = "protocol time", >> + .args_type = "protocol:s,time:s,display:s?", >> + .params = "protocol time [id=display]", >> .help = "set spice/vnc password expire-time", >> .cmd = hmp_expire_password, >> }, >> >> SRST >> -``expire_password [ vnc | spice ]`` *expire-time* >> - Specify when a password for spice/vnc becomes >> - invalid. *expire-time* accepts: >> +``expire_password [ vnc | spice ] expire-time [ id=display ]`` >> + Specify when a password for spice/vnc becomes invalid. >> + *display* behaves the same as in ``set_password``. >> + *expire-time* accepts: >> >> ``now`` >> Invalidate password instantly. >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >> index 31366e6331..30f5b2c3e3 100644 >> --- a/monitor/hmp-cmds.c >> +++ b/monitor/hmp-cmds.c >> @@ -1546,10 +1546,20 @@ void hmp_set_password(Monitor *mon, const QDict >> *qdict) >> { >> const char *protocol = qdict_get_str(qdict, "protocol"); >> const char *password = qdict_get_str(qdict, "password"); >> + const char *display = qdict_get_try_str(qdict, "display"); >> const char *connected = qdict_get_try_str(qdict, "connected"); >> Error *err = NULL; >> >> - qmp_set_password(protocol, password, !!connected, connected, &err); >> + if (display && strncmp(display, "id=", 3)) { >> + connected = display; >> + display = NULL; >> + } else if (display) { >> + /* skip "id=" */ >> + display = display + 3; >> + } >> + >> + qmp_set_password(protocol, password, !!connected, connected, >> !!display, >> + display, &err); >> hmp_handle_error(mon, err); >> } >> >> @@ -1557,9 +1567,15 @@ void hmp_expire_password(Monitor *mon, const QDict >> *qdict) >> { >> const char *protocol = qdict_get_str(qdict, "protocol"); >> const char *whenstr = qdict_get_str(qdict, "time"); >> + const char *display = qdict_get_try_str(qdict, "display"); >> Error *err = NULL; >> >> - qmp_expire_password(protocol, whenstr, &err); >> + if (display && !strncmp(display, "id=", 3)) { >> + /* skip "id=" */ >> + display = display + 3; >> + } >> + >> + qmp_expire_password(protocol, whenstr, !!display, display, &err); >> hmp_handle_error(mon, err); >> } >> >> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c >> index f7d64a6457..a9ded90a41 100644 >> --- a/monitor/qmp-cmds.c >> +++ b/monitor/qmp-cmds.c >> @@ -165,7 +165,8 @@ void qmp_system_wakeup(Error **errp) >> } >> >> void qmp_set_password(const char *protocol, const char *password, >> - bool has_connected, const char *connected, Error >> **errp) >> + bool has_connected, const char *connected, >> + bool has_display, const char *display, Error **errp) >> { >> int disconnect_if_connected = 0; >> int fail_if_connected = 0; >> @@ -198,7 +199,7 @@ void qmp_set_password(const char *protocol, const char >> *password, >> } >> /* Note that setting an empty password will not disable login >> through >> * this interface. */ >> - rc = vnc_display_password(NULL, password); >> + rc = vnc_display_password(has_display ? display : NULL, password); >> } else { >> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol", >> "'vnc' or 'spice'"); >> @@ -211,7 +212,7 @@ void qmp_set_password(const char *protocol, const char >> *password, >> } >> >> void qmp_expire_password(const char *protocol, const char *whenstr, >> - Error **errp) >> + bool has_display, const char *display, Error >> **errp) >> { >> time_t when; >> int rc; >> @@ -232,7 +233,7 @@ void qmp_expire_password(const char *protocol, const >> char *whenstr, >> } >> rc = qemu_spice.set_pw_expire(when); >> } else if (strcmp(protocol, "vnc") == 0) { >> - rc = vnc_display_pw_expire(NULL, when); >> + rc = vnc_display_pw_expire(has_display ? display : NULL, when); >> } else { >> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol", >> "'vnc' or 'spice'"); >> diff --git a/qapi/ui.json b/qapi/ui.json >> index 16bf03224f..24dca811f8 100644 >> --- a/qapi/ui.json >> +++ b/qapi/ui.json >> @@ -25,6 +25,9 @@ >> # 'disconnect' to disconnect existing clients >> # 'keep' to maintain existing clients >> # >> +# @display: In case of VNC, the id of the display where the password >> +# should be changed. Defaults to the first. >> +# >> # Returns: - Nothing on success >> # - If Spice is not enabled, DeviceNotFound >> # >> @@ -38,7 +41,8 @@ >> # >> ## >> { 'command': 'set_password', >> - 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} } >> + 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str', >> + '*display': 'str'} } >> >> ## >> # @expire_password: >> @@ -54,6 +58,9 @@ >> # - '+INT' where INT is the number of seconds from now (integer) >> # - 'INT' where INT is the absolute time in seconds >> # >> +# @display: In case of VNC, the id of the display where the password >> +# should be set to expire. Defaults to the first. >> +# >> # Returns: - Nothing on success >> # - If @protocol is 'spice' and Spice is not active, >> DeviceNotFound >> # >> @@ -71,7 +78,8 @@ >> # <- { "return": {} } >> # >> ## >> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': >> 'str'} } >> +{ 'command': 'expire_password', >> + 'data': {'protocol': 'str', 'time': 'str', '*display': 'str'} } >> >> ## >> # @screendump: >> -- >> 2.30.2 >> >> >> >> >