Re: [PATCH v5 11/24] ui/vnc: remove use of error_printf_unless_qmp()

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The error_printf_unless_qmp() will print to the monitor if the current
> one is HMP, if it is QMP nothing will be printed, otherwise stderr
> will be used.
>
> This scenario is easily handled by checking !monitor_cur_is_qmp() and
> then calling the error_printf() function.
>
> Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  ui/vnc.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index a61a4f937d..a209c32f6d 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3534,8 +3534,10 @@ int vnc_display_password(const char *id, const char *password)
>          return -EINVAL;
>      }
>      if (vd->auth == VNC_AUTH_NONE) {
> -        error_printf_unless_qmp("If you want use passwords please enable "
> -                                "password auth using '-vnc ${dpy},password'.\n");
> +        if (!monitor_cur_is_qmp()) {
> +            error_printf("If you want to use passwords, please enable "
> +                         "password auth using '-vnc ${dpy},password'.\n");
> +        }
>          return -EINVAL;
>      }
>  
> @@ -3574,9 +3576,11 @@ static void vnc_display_print_local_addr(VncDisplay *vd)
>          qapi_free_SocketAddress(addr);
>          return;
>      }
> -    error_printf_unless_qmp("VNC server running on %s:%s\n",
> -                            addr->u.inet.host,
> -                            addr->u.inet.port);
> +    if (!monitor_cur_is_qmp()) {
> +        error_printf("VNC server running on %s:%s\n",
> +                     addr->u.inet.host,
> +                     addr->u.inet.port);
> +    }
>      qapi_free_SocketAddress(addr);
>  }

These are the only remaining uses of monitor_cur_is_qmp() at the end of
this series.  Hmm.


The first hunk is arguably sub-par error reporting.
vnc_display_password() is called by qmp_set_password() and
qmp_change_vnc_password().

Here's HMP:

    (qemu) set_password vnc 1234
    If you want to use passwords, please enable password auth using '-vnc ${dpy},password'.
    Error: Could not set password


QMP is even worse:

    {"execute": "set_password", "arguments": {"protocol": "vnc", "password": "1"}}
    {"error": {"class": "GenericError", "desc": "Could not set password"}}

Better would be something like

    error_setg(errp, "VNC password authentication is disabled");
    error_append_hint(errp, "To enable it, use ...");

where ... is the preferred way to enable it (not sure it's -vnc).

HMP should then report

    Error: VNC password authentication is disabled
    To enable it, use ...

and QMP

    {"error": {"class": "GenericError", "desc": "VNC password authentication is disabled"}}

No need for monitor_cur_is_qmp() then.


The second hunk applies in vnc_display_print_local_addr(), called by
qemu_init() via qemu_init_displays(), vnc_init_func(), and
vnc_display_open().  Can't be QMP; the monitor_cur_is_qmp() check could
be dropped.

Better: move the call up the call chain until "can't be QMP" is
perfectly obvious.

By the way, vnc_display_open() could be static.