[v9,0/3] VNC-related HMP/QMP fixes

Message

Fiona Ebner Feb. 25, 2022, 8:49 a.m. UTC

Original cover letter by Stefan R.:

Since the removal of the generic 'qmp_change' command, one can no
longer replace the 'default' VNC display listen address at runtime
(AFAIK). For our users who need to set up a secondary VNC access port,
this means configuring a second VNC display (in addition to our
standard one for web-access), but it turns out one cannot set a
password on this second display at the moment, as the 'set_password'
call only operates on the 'default' display.

Additionally, using secret objects, the password is only read once at
startup. This could be considered a bug too, but is not touched in
this series and left for a later date.

v8 -> v9:
* use s instead of V to indicate when a flag takes a string parameter
* make @connected a common member of @SetPasswordOptions

v7 -> v8:
* drop last patch deprecating SetPasswordAction values besides 'keep'
  for VNC (unfortunately, I don't have enough time to try implementing
  'disconnect' and 'fail' for VNC in the near future)
* drop if conditionals for DisplayProtocol enum to make compilation
  with --disable-spice and/or --disable-vnc work
* order 'keep' first in enum, to fix how patch #3 uses it as an
  implicit default
* also set connected and has_connected for the VNC options in
  hmp_set_password
* fix typo in patch #1
* add missing '#' for description in patch #3

v6 -> v7:
* remove g_strdup and g_free, use strings directly
* squash in last patch

v5 -> v6:
* consider feedback from Markus' review, mainly:
  * fix crash bug in patch 1 (sorry, artifact of patch-splitting)
  * rely on '!has_param => param == NULL' to shorten code
  * add note to 'docs/about/deprecated.rst' and touch up comments a bit
* go back to g_free instead of qapi_free_* since the latter apparently tries to
  free the passed in pointer which lives on the stack...
* fix bug in HMP parsing (see patch 1)

v4 -> v5:
* add comment to patch 1 in "monitor-internal.h"
* use qapi_free_SetPasswordOptions and friends, don't leak strdups
* split QAPI change into 3 seperate patches

v3 -> v4:
* drop previously patch 1, this was fixed here instead:
  https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html
* patch 1: add Eric's R-b
* patch 2: remove if-assignment, use 'deprecated' feature in schema

v2 -> v3:
* refactor QMP schema for set/expire_password as suggested by Eric Blake and
  Markus Armbruster

v1 -> v2:
* add Marc-André's R-b on patch 1
* use '-d' flag as suggested by Eric Blake and Gerd Hoffmann
  * I didn't see a way to do this yet, so I added a "flags with values" arg type

Stefan Reiter (3):
  monitor/hmp: add support for flag argument with value
  qapi/monitor: refactor set/expire_password with enums
  qapi/monitor: allow VNC display id in set/expire_password

 hmp-commands.hx            |  24 ++++----
 monitor/hmp-cmds.c         |  47 ++++++++++++++-
 monitor/hmp.c              |  19 +++++-
 monitor/monitor-internal.h |   3 +-
 monitor/qmp-cmds.c         |  49 +++++----------
 qapi/ui.json               | 120 +++++++++++++++++++++++++++++++------
 6 files changed, 194 insertions(+), 68 deletions(-)

Comments

Dr. David Alan Gilbert March 2, 2022, 11:26 a.m. UTC | #1

* Fabian Ebner ([e-mail]) wrote:
> Original cover letter by Stefan R.:
> 
> Since the removal of the generic 'qmp_change' command, one can no
> longer replace the 'default' VNC display listen address at runtime
> (AFAIK). For our users who need to set up a secondary VNC access port,
> this means configuring a second VNC display (in addition to our
> standard one for web-access), but it turns out one cannot set a
> password on this second display at the moment, as the 'set_password'
> call only operates on the 'default' display.
> 
> Additionally, using secret objects, the password is only read once at
> startup. This could be considered a bug too, but is not touched in
> this series and left for a later date.

Queued

> v8 -> v9:
> * use s instead of V to indicate when a flag takes a string parameter
> * make @connected a common member of @SetPasswordOptions
> 
> v7 -> v8:
> * drop last patch deprecating SetPasswordAction values besides 'keep'
>   for VNC (unfortunately, I don't have enough time to try implementing
>   'disconnect' and 'fail' for VNC in the near future)
> * drop if conditionals for DisplayProtocol enum to make compilation
>   with --disable-spice and/or --disable-vnc work
> * order 'keep' first in enum, to fix how patch #3 uses it as an
>   implicit default
> * also set connected and has_connected for the VNC options in
>   hmp_set_password
> * fix typo in patch #1
> * add missing '#' for description in patch #3
> 
> v6 -> v7:
> * remove g_strdup and g_free, use strings directly
> * squash in last patch
> 
> v5 -> v6:
> * consider feedback from Markus' review, mainly:
>   * fix crash bug in patch 1 (sorry, artifact of patch-splitting)
>   * rely on '!has_param => param == NULL' to shorten code
>   * add note to 'docs/about/deprecated.rst' and touch up comments a bit
> * go back to g_free instead of qapi_free_* since the latter apparently tries to
>   free the passed in pointer which lives on the stack...
> * fix bug in HMP parsing (see patch 1)
> 
> v4 -> v5:
> * add comment to patch 1 in "monitor-internal.h"
> * use qapi_free_SetPasswordOptions and friends, don't leak strdups
> * split QAPI change into 3 seperate patches
> 
> v3 -> v4:
> * drop previously patch 1, this was fixed here instead:
>   https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html
> * patch 1: add Eric's R-b
> * patch 2: remove if-assignment, use 'deprecated' feature in schema
> 
> v2 -> v3:
> * refactor QMP schema for set/expire_password as suggested by Eric Blake and
>   Markus Armbruster
> 
> v1 -> v2:
> * add Marc-André's R-b on patch 1
> * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann
>   * I didn't see a way to do this yet, so I added a "flags with values" arg type
> 
> Stefan Reiter (3):
>   monitor/hmp: add support for flag argument with value
>   qapi/monitor: refactor set/expire_password with enums
>   qapi/monitor: allow VNC display id in set/expire_password
> 
>  hmp-commands.hx            |  24 ++++----
>  monitor/hmp-cmds.c         |  47 ++++++++++++++-
>  monitor/hmp.c              |  19 +++++-
>  monitor/monitor-internal.h |   3 +-
>  monitor/qmp-cmds.c         |  49 +++++----------
>  qapi/ui.json               | 120 +++++++++++++++++++++++++++++++------
>  6 files changed, 194 insertions(+), 68 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
>