Re: [RFC PATCH 6/6] vl: introduce -audio as a replacement for -soundhw

[Martin Kletzander <mkletzan@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On Wed, Apr 27, 2022 at 01:32:25PM +0200, Paolo Bonzini wrote:
>-audio is used like "-audio pa,model=sb16".  It is almost as simple as
>-soundhw, but it reuses the -audiodev parsing machinery and attaches an
>audiodev to the newly-created device.  The main 'feature' is that
>it knows about adding the codec device for model=intel-hda, and adding
>the audiodev to the codec device.
>
>In the future, it could be extended to support default models or
>builtin devices, just like -nic, or even a default backend.  For now,
>keep it simple.
>
>JSON parsing is not supported for -audio.  This is okay because the
>option is targeted at end users, not programs.
>
>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>---
> audio/audio.c                   |  8 +++++-
> audio/audio.h                   |  1 +
> docs/about/deprecated.rst       |  9 ------
> docs/about/removed-features.rst |  7 +++++
> hw/audio/intel-hda.c            |  5 ++--
> hw/audio/soundhw.c              | 12 +++++---
> include/hw/audio/soundhw.h      |  4 +--
> qemu-options.hx                 | 51 ++++++++++++++++-----------------
> softmmu/vl.c                    | 28 ++++++++++++++++--
> 9 files changed, 76 insertions(+), 49 deletions(-)
>

[...]

>diff --git a/softmmu/vl.c b/softmmu/vl.c
>index 5bea0eb3eb..979bbda5aa 100644
>--- a/softmmu/vl.c
>+++ b/softmmu/vl.c
>@@ -3018,13 +3020,33 @@ void qemu_init(int argc, char **argv, char **envp)
>             case QEMU_OPTION_audiodev:
>                 audio_parse_option(optarg);
>                 break;
>-            case QEMU_OPTION_soundhw:
>-                if (is_help_option(optarg)) {
>+            case QEMU_OPTION_audio: {
>+                QDict *dict = keyval_parse(optarg, "driver", NULL, &error_fatal);
>+                char *model;
>+                Audiodev *dev = NULL;
>+                Visitor *v;
>+
>+                if (!qdict_haskey(dict, "id")) {
>+                    qdict_put_str(dict, "id", "audiodev0");
>+                }
>+                if (!qdict_haskey(dict, "model")) {
>+                    error_setg(&error_fatal, "Parameter 'model' is missing");
>+                }
>+                model = g_strdup(qdict_get_str(dict, "model"));
>+                qdict_del(dict, "model");
>+                if (is_help_option(model)) {
>                     show_valid_soundhw();
>                     exit(0);
>                 }
>-                select_soundhw (optarg);
>+                v = qobject_input_visitor_new_keyval(QOBJECT(dict));
>+                qobject_unref(dict);
>+                visit_type_Audiodev(v, NULL, &dev, &error_fatal);
>+                visit_free(v);
>+                audio_define(dev);

I was looking at this and thought that you might be creating multiple
audiodevs if there are multiple -audio options.  And I found out that
even with current master it is possible to create multiple audiodevs not
only with the same driver, but even with the same ID (and different
drivers).

I guess that is not preferable, but I can't say for sure how this is
supposed to be handled.

Other than that it looks fine to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]