Re: [PATCH] audio: exit(1) if audio backend failed to be found or initialized

[Marc-André Lureau]

Hi

On Tue, Aug 23, 2022 at 10:53 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> From: Marc-André Lureau<marcandre.lureau@redhat.com>
>
> If you specify a known backend but it isn't compiled in, or failed to
> initialize, you get a simple warning and the "none" backend as a
> fallback, and QEMU runs happily:
>
> $ qemu-system-x86_64 -audiodev id=audio,driver=dsound
> audio: Unknown audio driver `dsound'
> audio: warning: Using timer based audio emulation
> ...
>
> Instead, QEMU should fail to start:
> $ qemu-system-x86_64 -audiodev id=audio,driver=dsound
> audio: Unknown audio driver `dsound'
> $
>
> Resolves:
> https://bugzilla.redhat.com/show_bug.cgi?id=1983493
>
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>
> ---
>   audio/audio.h |  2 +-
>   audio/audio.c | 14 +++++++++++---
>   softmmu/vl.c  |  4 +++-
>   3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/audio/audio.h b/audio/audio.h
> index b5e17cd218..27e67079a0 100644
> --- a/audio/audio.h
> +++ b/audio/audio.h
> @@ -170,7 +170,7 @@ void audio_sample_from_uint64(void *samples, int pos,
>   
>   void audio_define(Audiodev *audio);
>   void audio_parse_option(const char *opt);
> -void audio_init_audiodevs(void);
> +bool audio_init_audiodevs(void);
>   void audio_legacy_help(void);
>   
>   AudioState *audio_state_by_name(const char *name);
> diff --git a/audio/audio.c b/audio/audio.c
> index a02f3ce5c6..76b8735b44 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1743,7 +1743,6 @@ static AudioState *audio_init(Audiodev *dev, const char *name)
>           atexit(audio_cleanup);
>           atexit_registered = true;
>       }
> -    QTAILQ_INSERT_TAIL(&audio_states, s, list);
>   
>       s->ts = timer_new_ns(QEMU_CLOCK_VIRTUAL, audio_timer, s);
>   
> @@ -1769,6 +1768,10 @@ static AudioState *audio_init(Audiodev *dev, const char *name)
>           } else {
>               dolog ("Unknown audio driver `%s'\n", drvname);
>           }
> +        if (!done) {
> +            free_audio_state(s);
> +            return NULL;

Hi Marc-André,

I don't understand why you move the QTAILQ_INSERT_TAIL(&audio_states, s,
list) macro down. Without this you don't need the additional

free_audio_state() call. audio_cleanup() takes care to free the audio state.

Ah I see what you mean, atexit will be called. But since exit() is called outside the function, it's not a good idea to rely on the caller imho. Better not add it at all imho, and free state on failure.

 

 

The rest looks good. You can add my

Reviewed-by: Volker Rümelin <vr_qemu@t-online.de> 

even if you insist on keeping the patch as it is.

thanks

 

With best regards,
Volker

> +        }
>       } else {
>           for (i = 0; audio_prio_list[i]; i++) {
>               AudiodevListEntry *e = audiodev_find(&head, audio_prio_list[i]);
> @@ -1806,6 +1809,7 @@ static AudioState *audio_init(Audiodev *dev, const char *name)
>                  "(Audio can continue looping even after stopping the VM)\n");
>       }
>   
> +    QTAILQ_INSERT_TAIL(&audio_states, s, list);
>       QLIST_INIT (&s->card_head);
>       vmstate_register (NULL, 0, &vmstate_audio, s);
>       return s;
> @@ -2119,13 +2123,17 @@ void audio_define(Audiodev *dev)
>       QSIMPLEQ_INSERT_TAIL(&audiodevs, e, next);
>   }
>   
> -void audio_init_audiodevs(void)
> +bool audio_init_audiodevs(void)
>   {
>       AudiodevListEntry *e;
>   
>       QSIMPLEQ_FOREACH(e, &audiodevs, next) {
> -        audio_init(e->dev, NULL);
> +        if (!audio_init(e->dev, NULL)) {
> +            return false;
> +        }
>       }
> +
> +    return true;
>   }
>   
>   audsettings audiodev_to_audsettings(AudiodevPerDirectionOptions *pdo)
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 706bd7cff7..dea4005e47 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1885,7 +1885,9 @@ static void qemu_create_early_backends(void)
>        * setting machine properties, so they can be referred to.
>        */
>       configure_blockdev(&bdo_queue, machine_class, snapshot);
> -    audio_init_audiodevs();
> +    if (!audio_init_audiodevs()) {
> +        exit(1);
> +    }
>   }
>   
>   

--

Marc-André Lureau