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

From: "Zoltán Kővágó" <dirty.ice.hu@gmail.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	qemu-devel@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 06/52] audio: -audiodev command line option basic implementation
Date: Thu, 10 Jan 2019 01:13:21 +0100	[thread overview]
Message-ID: <6619c58a-3e5a-a302-9bf0-acac6f7870e9@gmail.com> (raw)
In-Reply-To: <87va2zg6hh.fsf@dusky.pond.sub.org>

On 2019-01-08 04:42, Markus Armbruster wrote:
> "Zoltán Kővágó" <dirty.ice.hu@gmail.com> writes:
> 
>> On 2019-01-07 14:13, Markus Armbruster wrote:
>>> "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.
>>>
>>
>> Yes, this patch series is a bit old, and at that time this was the best
>> I could do. I can look into it this (probably only on the weekend
>> though), it looks like it supports foo.bar=something syntax, so I don't
>> have to specify a json on the command line...
> 
> The dotted key syntax and how it relates to JSON is documented in
> util/keyval.c.
> 
> If you have further questions on this stuff, please don't hesitate to
> ask me.
> 
> If you get to the point where you feel your (beefy) series has gotten
> stuck on the conversion away from the options visitor, let's discuss how
> to best get it unstuck.
> 
> Thanks!
> 

I briefly looked at converting this single patch, and of course I ran
into a few issues.

The audio_legacy code currently creates a QemuOpts from the environment
variables.  This had a nice effect that once I converted the options, I
could print them with qemu_opts_print, helping the user how to convert
the old env variables to -audiodev, since some of the conversions are
not so trivial/straightforward.

I converted them to QDict, which complicated things a bit since now I
have to do proper nesting (until now I could just set key to "foo.bar"
and it worked), especially in the *_to_usecs functions that accessed
previously set options.  Also while there's
qobject_input_visitor_new_str, I couldn't find anything that would do
the reverse.  I wrote a quick and dirty function that does this, it
appears to work, but unlike QemuOpts, QDict is unordered, and writing
-audiodev id=foo,driver=alsa,rest... is IMHO more logical than writing
them alphabetically.  Or maybe it's just me who's too picky and it
doesn't matter.

I was thinking about creating an Audiodev (the qapi type) directly would
be better, then somehow print it with reflection.  While this is not a
typical use of qapi, at least qmp_qom_list creates qapi objects
directly, so I assume it's ok.  This would save us from parsing
essentially the same data twice, and the code to deal with an Audiodev
is simpler that dealing with the generic QDicts and QObjects, but it
would require some refactoring.

The second problem: with my patched options visitor, nested structs were
required in qapi, the options visitor unconditionally filled them.  With
qobject_input_visitor, I have to mark them optional, otherwise the user
have to specify at least one option from the nested structs.

Essentially, if you have something like this:
{ 'struct': 'Inner', 'data': [ '*foo': 'str' ] },
{ 'struct': 'Outer', 'data': [ '*bar': 'Inner' ] }
With the old parser you couldn't specify {}, while with
qobject_input_visitor_new_str, you can's specify { 'bar': {} } without
using JSON.  I'm not sure which is the better, but with the old behavior
at least you could mark nested structs as required and not worry about
checking has_x every time.

Now that I wrote all this down, I'm starting to think that
qobject_input_visitor is good, and I should just properly check the
fields (or give them default values).  Rubber duck debugging, I think.

Regards,
Zoltan