[PATCH v3 02/11] ui/vnc: disable audio feature when configured with --disable-audio

Sergei Heifetz posted 11 patches 3 weeks, 1 day ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Dr. David Alan Gilbert" <dave@treblig.org>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>
[PATCH v3 02/11] ui/vnc: disable audio feature when configured with --disable-audio
Posted by Sergei Heifetz 3 weeks, 1 day ago
Disable the audio feature in VNC when QEMU is configured with
`--disable-audio`. Do not compile the corresponding audio-related
code.

The audio feature is disabled in a way that makes a QEMU server behave
like a non-QEMU server (as far as audio is concerned). The client sends
the audio pseudo-encoding, but the server does not respond with
confirmation, because send_ext_audio_ack is now behind an #ifdef.
A well-behaved client should handle this accordingly and continue
using VNC without sending any audio-related messages to the server.
If a client misbehaves and sends VNC_MSG_CLIENT_QEMU_AUDIO while
the audio feature is disabled, the server will close the connection with
an error.

The behaviour described above is already present when there is no audiodev
(`vs->vd->audio_be` is NULL).

Signed-off-by: Sergei Heifetz <heifetz@yandex-team.com>
---
 qemu-options.hx |  3 +++
 ui/vnc.c        | 27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 890c4f1d230..04ea2239296 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2723,6 +2723,9 @@ SRST
         must be omitted, otherwise is must be present and specify a
         valid audiodev.
 
+        If QEMU is configured with ``--disable-audio``, this parameter
+        is not present and audio is not transmitted over VNC.
+
     ``power-control=on|off``
         Permit the remote client to issue shutdown, reboot or reset power
         control requests.
diff --git a/ui/vnc.c b/ui/vnc.c
index 952976e9649..0ee5502d1e6 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1072,6 +1072,7 @@ static void vnc_update_throttle_offset(VncState *vs)
     size_t offset =
         vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
 
+#ifdef CONFIG_AUDIO
     if (vs->audio_cap) {
         int bps;
         switch (vs->as.fmt) {
@@ -1091,6 +1092,7 @@ static void vnc_update_throttle_offset(VncState *vs)
         }
         offset += vs->as.freq * bps * vs->as.nchannels;
     }
+#endif
 
     /* Put a floor of 1MB on offset, so that if we have a large pending
      * buffer and the display is resized to a small size & back again
@@ -1214,6 +1216,7 @@ static int vnc_update_client(VncState *vs, int has_dirty)
     return n;
 }
 
+#ifdef CONFIG_AUDIO
 /* audio */
 static void audio_capture_notify(void *opaque, audcnotification_e cmd)
 {
@@ -1293,6 +1296,7 @@ static void audio_del(VncState *vs)
         vs->audio_cap = NULL;
     }
 }
+#endif
 
 static void vnc_disconnect_start(VncState *vs)
 {
@@ -1332,7 +1336,9 @@ void vnc_disconnect_finish(VncState *vs)
 #ifdef CONFIG_VNC_SASL
     vnc_sasl_client_cleanup(vs);
 #endif /* CONFIG_VNC_SASL */
+#ifdef CONFIG_AUDIO
     audio_del(vs);
+#endif
     qkbd_state_lift_all_keys(vs->vd->kbd);
 
     if (vs->mouse_mode_notifier.notify != NULL) {
@@ -2097,6 +2103,7 @@ static void send_ext_key_event_ack(VncState *vs)
     vnc_flush(vs);
 }
 
+#ifdef CONFIG_AUDIO
 static void send_ext_audio_ack(VncState *vs)
 {
     vnc_lock_output(vs);
@@ -2110,6 +2117,7 @@ static void send_ext_audio_ack(VncState *vs)
     vnc_unlock_output(vs);
     vnc_flush(vs);
 }
+#endif
 
 static void send_xvp_message(VncState *vs, int code)
 {
@@ -2197,10 +2205,15 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
             send_ext_key_event_ack(vs);
             break;
         case VNC_ENCODING_AUDIO:
+#ifdef CONFIG_AUDIO
             if (vs->vd->audio_be) {
                 vnc_set_feature(vs, VNC_FEATURE_AUDIO);
                 send_ext_audio_ack(vs);
             }
+#else
+            VNC_DEBUG("Audio encoding received with audio subsystem "
+                      "disabled\n");
+#endif
             break;
         case VNC_ENCODING_WMVi:
             vnc_set_feature(vs, VNC_FEATURE_WMVI);
@@ -2394,7 +2407,9 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
 {
     int i;
     uint16_t limit;
+#ifdef CONFIG_AUDIO
     uint32_t freq;
+#endif
     VncDisplay *vd = vs->vd;
 
     if (data[0] > 3) {
@@ -2572,6 +2587,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
                 break;
             }
 
+#ifdef CONFIG_AUDIO
             if (len == 2)
                 return 4;
 
@@ -2626,6 +2642,9 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
                 break;
             }
             break;
+#else
+            abort();
+#endif
 
         default:
             VNC_DEBUG("Msg: %d\n", read_u16(data, 0));
@@ -3369,10 +3388,12 @@ static void vnc_connect(VncDisplay *vd, QIOChannelSocket *sioc,
     vs->last_x = -1;
     vs->last_y = -1;
 
+#ifdef CONFIG_AUDIO
     vs->as.freq = 44100;
     vs->as.nchannels = 2;
     vs->as.fmt = AUDIO_FORMAT_S16;
     vs->as.big_endian = false;
+#endif
 
     qemu_mutex_init(&vs->output_mutex);
     vs->bh = qemu_bh_new(vnc_jobs_bh, vs);
@@ -3649,9 +3670,11 @@ static QemuOptsList qemu_vnc_opts = {
         },{
             .name = "non-adaptive",
             .type = QEMU_OPT_BOOL,
+#ifdef CONFIG_AUDIO
         },{
             .name = "audiodev",
             .type = QEMU_OPT_STRING,
+#endif
         },{
             .name = "power-control",
             .type = QEMU_OPT_BOOL,
@@ -4084,7 +4107,9 @@ void vnc_display_open(const char *id, Error **errp)
     const char *saslauthz;
     int lock_key_sync = 1;
     int key_delay_ms;
+#ifdef CONFIG_AUDIO
     const char *audiodev;
+#endif
     const char *passwordSecret;
 
     if (!vd) {
@@ -4242,6 +4267,7 @@ void vnc_display_open(const char *id, Error **errp)
     }
     vd->ledstate = 0;
 
+#ifdef CONFIG_AUDIO
     audiodev = qemu_opt_get(opts, "audiodev");
     if (audiodev) {
         vd->audio_be = audio_be_by_name(audiodev, errp);
@@ -4251,6 +4277,7 @@ void vnc_display_open(const char *id, Error **errp)
     } else {
         vd->audio_be = audio_get_default_audio_be(NULL);
     }
+#endif
 
     device_id = qemu_opt_get(opts, "display");
     if (device_id) {
-- 
2.34.1
Re: [PATCH v3 02/11] ui/vnc: disable audio feature when configured with --disable-audio
Posted by Philippe Mathieu-Daudé 5 days, 20 hours ago
Hi,

On 15/3/26 21:16, Sergei Heifetz wrote:
> Disable the audio feature in VNC when QEMU is configured with
> `--disable-audio`. Do not compile the corresponding audio-related
> code.
> 
> The audio feature is disabled in a way that makes a QEMU server behave
> like a non-QEMU server (as far as audio is concerned). The client sends
> the audio pseudo-encoding, but the server does not respond with
> confirmation, because send_ext_audio_ack is now behind an #ifdef.
> A well-behaved client should handle this accordingly and continue
> using VNC without sending any audio-related messages to the server.
> If a client misbehaves and sends VNC_MSG_CLIENT_QEMU_AUDIO while
> the audio feature is disabled, the server will close the connection with
> an error.
> 
> The behaviour described above is already present when there is no audiodev
> (`vs->vd->audio_be` is NULL).
> 
> Signed-off-by: Sergei Heifetz <heifetz@yandex-team.com>
> ---
>   qemu-options.hx |  3 +++
>   ui/vnc.c        | 27 +++++++++++++++++++++++++++
>   2 files changed, 30 insertions(+)


> @@ -2197,10 +2205,15 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
>               send_ext_key_event_ack(vs);
>               break;
>           case VNC_ENCODING_AUDIO:
> +#ifdef CONFIG_AUDIO
>               if (vs->vd->audio_be) {
>                   vnc_set_feature(vs, VNC_FEATURE_AUDIO);
>                   send_ext_audio_ack(vs);
>               }
> +#else
> +            VNC_DEBUG("Audio encoding received with audio subsystem "
> +                      "disabled\n");
> +#endif
>               break;

IIRC the way the project choose to compartmentalize subsystem is
to use stubs instead of cluttering the source with #ifdef'ry, so
I'd expect this effort to be extracting the audio code. For example
here declare vnc_audio_set_encodings() in "ui/vnc.h" ...:

   void vnc_audio_set_encodings(VncState *vs,
                                int32_t *encodings, size_t n_encodings);

... move the current definition to ui/vnc-audio.c ...:

   void vnc_audio_set_encodings(VncState *vs,
                                int32_t *encodings, size_t n_encodings)
   {
       if (!vs->vd->audio_be) {
           return;
       }
       vnc_set_feature(vs, VNC_FEATURE_AUDIO);
       send_ext_audio_ack(vs);
   }

... and the stub in ui/vnc-audio-stub.c:

   void vnc_audio_set_encodings(VncState *vs,
                                int32_t *encodings, size_t n_encodings)
   {
       VNC_DEBUG("Audio encoding received with audio subsystem "
                 "disabled\n");
   }

(Although VNC_DEBUG calls fprintf so better would be to convert to
  error_report in a preliminary cleanup patch).

and the ui/meson.build change becomes:

   vnc_ss.add(when: 'CONFIG_AUDIO', if_true: files('vnc-audio.c'))
   stub_ss.add(files('vnc-audio-stub.c'))

Does it make sense?

Regards,

Phil.
Re: [PATCH v3 02/11] ui/vnc: disable audio feature when configured with --disable-audio
Posted by Sergei Heifetz 5 days, 8 hours ago
On Wed Apr 1, 2026 at 3:07 PM +05, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 15/3/26 21:16, Sergei Heifetz wrote:
>> Disable the audio feature in VNC when QEMU is configured with
>> `--disable-audio`. Do not compile the corresponding audio-related
>> code.
>> 
>> The audio feature is disabled in a way that makes a QEMU server behave
>> like a non-QEMU server (as far as audio is concerned). The client sends
>> the audio pseudo-encoding, but the server does not respond with
>> confirmation, because send_ext_audio_ack is now behind an #ifdef.
>> A well-behaved client should handle this accordingly and continue
>> using VNC without sending any audio-related messages to the server.
>> If a client misbehaves and sends VNC_MSG_CLIENT_QEMU_AUDIO while
>> the audio feature is disabled, the server will close the connection with
>> an error.
>> 
>> The behaviour described above is already present when there is no audiodev
>> (`vs->vd->audio_be` is NULL).
>> 
>> Signed-off-by: Sergei Heifetz <heifetz@yandex-team.com>
>> ---
>>   qemu-options.hx |  3 +++
>>   ui/vnc.c        | 27 +++++++++++++++++++++++++++
>>   2 files changed, 30 insertions(+)
>
>
>> @@ -2197,10 +2205,15 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
>>               send_ext_key_event_ack(vs);
>>               break;
>>           case VNC_ENCODING_AUDIO:
>> +#ifdef CONFIG_AUDIO
>>               if (vs->vd->audio_be) {
>>                   vnc_set_feature(vs, VNC_FEATURE_AUDIO);
>>                   send_ext_audio_ack(vs);
>>               }
>> +#else
>> +            VNC_DEBUG("Audio encoding received with audio subsystem "
>> +                      "disabled\n");
>> +#endif
>>               break;
>
> IIRC the way the project choose to compartmentalize subsystem is
> to use stubs instead of cluttering the source with #ifdef'ry, so
> I'd expect this effort to be extracting the audio code. For example
> here declare vnc_audio_set_encodings() in "ui/vnc.h" ...:
>
> [...]
>
> Does it make sense?

Has the project really chosen this approach? Does it apply only to
subsystems, and not to individual options? I did it without stubs
because it's simpler, and there is already a lot of #ifdef'ry present in
the code. I am, of course, willing to refactor the series to use stubs
if it's necessary, but I wonder if you could provide a bit more context
about why you think it should be done.

Also, how strict do we want to be about it? There are places where the
stub approach looks perfect, but there are also places where, for
example, the whole change is putting a few lines of a big function
inside an #ifdef block. Do we want to be as strict as possible, or try
to strike a balance?