Re: [Qemu-devel] [PATCH v2 06/52] audio: -audiodev command line option basic implementation

[Markus Armbruster <armbru@redhat.com>]

"Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes:

> Audio drivers now get an Audiodev * as config paramters, instead of the
> global audio_option structs.  There is some code in audio/audio_legacy.c
> that converts the old environment variables to audiodev options (this
> way backends do not have to worry about legacy options).  It also
> contains a replacement of -audio-help, which prints out the equivalent
> -audiodev based config of the currently specified environment variables.
>
> Note that backends are not updated and still rely on environment
> variables.
>
> Also note that (due to moving try-poll from global to backend specific
> option) currently ALSA and OSS will always try poll mode, regardless of
> environment variables or -audiodev options.
>
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> ---
[...]
> diff --git a/audio/audio.c b/audio/audio.c
> index 96cbd57c37..e7f25ea84b 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
[...]
> @@ -2127,3 +1841,158 @@ void AUD_set_volume_in (SWVoiceIn *sw, int mute, uint8_t lvol, uint8_t rvol)
>          }
>      }
>  }
> +
> +QemuOptsList qemu_audiodev_opts = {
> +    .name = "audiodev",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_audiodev_opts.head),
> +    .implied_opt_name = "driver",
> +    .desc = {
> +        /*
> +         * no elements => accept any params
> +         * sanity checking will happen later
> +         */
> +        { /* end of list */ }
> +    },
> +};
> +
> +static void validate_per_direction_opts(AudiodevPerDirectionOptions *pdo,
> +                                        Error **errp)
> +{
> +    if (!pdo->has_fixed_settings) {
> +        pdo->has_fixed_settings = true;
> +        pdo->fixed_settings = true;
> +    }
> +    if (!pdo->fixed_settings &&
> +        (pdo->has_frequency || pdo->has_channels || pdo->has_format)) {
> +        error_setg(errp,
> +                   "You can't use frequency, channels or format with fixed-settings=off");
> +        return;
> +    }
> +
> +    if (!pdo->has_frequency) {
> +        pdo->has_frequency = true;
> +        pdo->frequency = 44100;
> +    }
> +    if (!pdo->has_channels) {
> +        pdo->has_channels = true;
> +        pdo->channels = 2;
> +    }
> +    if (!pdo->has_voices) {
> +        pdo->has_voices = true;
> +        pdo->voices = 1;
> +    }
> +    if (!pdo->has_format) {
> +        pdo->has_format = true;
> +        pdo->format = AUDIO_FORMAT_S16;
> +    }
> +}
> +
> +static Audiodev *parse_option(QemuOpts *opts, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    Visitor *v = opts_visitor_new(opts, true);
> +    Audiodev *dev = NULL;
> +    visit_type_Audiodev(v, NULL, &dev, &local_err);
> +    visit_free(v);
> +
> +    if (local_err) {
> +        goto err2;
> +    }
> +
> +    validate_per_direction_opts(dev->in, &local_err);
> +    if (local_err) {
> +        goto err;
> +    }
> +    validate_per_direction_opts(dev->out, &local_err);
> +    if (local_err) {
> +        goto err;
> +    }
> +
> +    if (!dev->has_timer_period) {
> +        dev->has_timer_period = true;
> +        dev->timer_period = 10000; /* 100Hz -> 10ms */
> +    }
> +
> +    return dev;
> +
> +err:
> +    qapi_free_Audiodev(dev);
> +err2:
> +    error_propagate(errp, local_err);
> +    return NULL;
> +}
> +
> +static int each_option(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    Audiodev *dev = parse_option(opts, errp);
> +    if (!dev) {
> +        return -1;
> +    }
> +    return audio_init(dev);
> +}
> +
> +void audio_set_options(void)
> +{
> +    if (qemu_opts_foreach(qemu_find_opts("audiodev"), each_option, NULL,
> +                          &error_abort)) {
> +        exit(1);
> +    }
> +}
[...]
> diff --git a/vl.c b/vl.c
> index 8353d3c718..b5364ffe46 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3074,6 +3074,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_option_rom_opts);
>      qemu_add_opts(&qemu_machine_opts);
>      qemu_add_opts(&qemu_accel_opts);
> +    qemu_add_opts(&qemu_audiodev_opts);
>      qemu_add_opts(&qemu_mem_opts);
>      qemu_add_opts(&qemu_smp_opts);
>      qemu_add_opts(&qemu_boot_opts);
> @@ -3307,9 +3308,15 @@ int main(int argc, char **argv, char **envp)
>                  add_device_config(DEV_BT, optarg);
>                  break;
>              case QEMU_OPTION_audio_help:
> -                AUD_help ();
> +                audio_legacy_help();
>                  exit (0);
>                  break;
> +            case QEMU_OPTION_audiodev:
> +                if (!qemu_opts_parse_noisily(qemu_find_opts("audiodev"),
> +                                             optarg, true)) {
> +                    exit(1);
> +                }
> +                break;
>              case QEMU_OPTION_soundhw:
>                  select_soundhw (optarg);
>                  break;
> @@ -4545,6 +4552,8 @@ int main(int argc, char **argv, char **envp)
>      /* do monitor/qmp handling at preconfig state if requested */
>      main_loop();
>  
> +    audio_set_options();
> +
>      /* from here on runstate is RUN_STATE_PRELAUNCH */
>      machine_run_board_init(current_machine);

This uses the options visitor, roughly like -netdev.  Depends on your
options visitor enhancements.  Is there any particular reason not to do
it like -blockdev and -display, with qobject_input_visitor_new_str()?
I'm asking because I'd very much like new code to use
qobject_input_visitor_new_str() rather than the options visitor.

Today, the options visitor looks like an evolutionary dead end.  My
command-line qapification (still stuck at the RFC stage, hope to get it
unstuck this year) relies on qobject_input_visitor_new_str().  The goal
is to convert *all* of the command line to it.

I suspect your series uses the options visitor only because back when
you started, qobject_input_visitor_new_str() didn't exist.