Re: [PATCH 01/13] ui/vnc: Require audiodev= to enable audio

[Daniel P . Berrangé]

On Sat, Sep 23, 2023 at 10:54:54AM +0200, Paolo Bonzini wrote:
> From: Martin Kletzander <mkletzan@redhat.com>
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> Message-ID: 
> <a07513f1bf6123fef52ff5e7943f5704746b376b.1650874791.git.mkletzan@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  docs/about/deprecated.rst       |  8 +++-----
>  docs/about/removed-features.rst |  6 ++++++
>  ui/vnc.c                        | 10 ++++++++--
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 8f3fef97bd4..c07bf58dde1 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -45,13 +45,11 @@ backend settings instead of environment variables.  To 
> ease migration to
>  the new format, the ``-audiodev-help`` option can be used to convert
>  the current values of the environment variables to ``-audiodev`` options.
>  
> -Creating sound card devices and vnc without ``audiodev=`` property (since 
> 4.2)
> -''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +Creating sound card devices without ``audiodev=`` property (since 4.2)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>  
>  When not using the deprecated legacy audio config, each sound card
> -should specify an ``audiodev=`` property.  Additionally, when using
> -vnc, you should specify an ``audiodev=`` property if you plan to
> -transmit audio through the VNC protocol.
> +should specify an ``audiodev=`` property.
>  
>  Short-form boolean options (since 6.0)
>  ''''''''''''''''''''''''''''''''''''''
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index 97ec47f1d25..276060b320c 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -436,6 +436,12 @@ the process listing. This was replaced by the new 
> ``password-secret``
>  option which lets the password be securely provided on the command
>  line using a ``secret`` object instance.
>  
> +Creating vnc without ``audiodev=`` property (removed in 8.2)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +When using vnc, you should specify an ``audiodev=`` property if
> +you plan to transmit audio through the VNC protocol.
> +
>  QEMU Machine Protocol (QMP) commands
>  ------------------------------------
>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 6fd86996a54..cfa18bbd3e1 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2508,11 +2508,17 @@ static int protocol_client_msg(VncState *vs, uint8_t 
> *data, size_t len)
>              switch (read_u16 (data, 2)) {
>              case VNC_MSG_CLIENT_QEMU_AUDIO_ENABLE:
>                  trace_vnc_msg_client_audio_enable(vs, vs->ioc);
> -                audio_add(vs);
> +                if (vs->vd->audio_state) {
> +                    audio_add(vs);
> +                } else {
> +                    error_report("audio not available, use audiodev= option 
> for vnc");
> +                }
>                  break;
>              case VNC_MSG_CLIENT_QEMU_AUDIO_DISABLE:
>                  trace_vnc_msg_client_audio_disable(vs, vs->ioc);
> -                audio_del(vs);
> +                if (vs->vd->audio_state) {
> +                    audio_del(vs);
> +                }
>                  break;
>              case VNC_MSG_CLIENT_QEMU_AUDIO_SET_FORMAT:
>                  if (len == 4)

This is not good. The code is still advertizing the audio encoding
to client despite the fact it will not work. This will result in
spamming the console every time an audio-enabled VNC client connects.

We need to *not* send the audio ack in response to VNC_ENCODING_AUDIO,
so that clients aren't told audio exists.

In protocol_client_msg we need to call vnc_client_error() to immediately
drop the client if they try to send any audio control messages when
audio is not advertized.

With 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 :|