Re: [PATCH 07/18] Introduce machine's default-audiodev property

[Martin Kletzander <mkletzan@redhat.com>]

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

On Mon, Apr 25, 2022 at 03:06:14PM +0100, Daniel P. Berrangé wrote:
>On Mon, Apr 25, 2022 at 10:21:50AM +0200, Martin Kletzander wrote:
>> Many machine types have default audio devices with no way to set the underlying
>> audiodev.  Instead of adding an option for each and every one of them this new
>> property can be used as a default during machine initialisation when creating
>> such devices.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  hw/core/machine.c   | 23 +++++++++++++++++++++++
>>  include/hw/boards.h |  1 +
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index cb9bbc844d24..d055a126d398 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -596,6 +596,22 @@ static void machine_set_memdev(Object *obj, const char *value, Error **errp)
>>      ms->ram_memdev_id = g_strdup(value);
>>  }
>>
>> +static char *machine_get_default_audiodev(Object *obj, Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(obj);
>> +
>> +    return g_strdup(ms->default_audiodev);
>> +}
>> +
>> +static void machine_set_default_audiodev(Object *obj, const char *value,
>> +                                         Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(obj);
>> +
>> +    g_free(ms->default_audiodev);
>> +    ms->default_audiodev = g_strdup(value);
>> +}
>> +
>>  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
>>  {
>>      int i;
>> @@ -867,6 +883,12 @@ static void machine_class_init(ObjectClass *oc, void *data)
>>      object_class_property_set_description(oc, "confidential-guest-support",
>>                                            "Set confidential guest scheme to support");
>>
>> +    object_class_property_add_str(oc, "default-audiodev",
>> +                                  machine_get_default_audiodev,
>> +                                  machine_set_default_audiodev);
>> +    object_class_property_set_description(oc, "default-audiodev",
>> +                                          "Audiodev to use for default machine devices");
>> +
>
>Hmm, if we add this, people might reasonably question why this default
>audiodev can't be used for everything, instead of making other 'audiodev'
>parameter mandatory.
>
>For the x86 machines we have a property tied specifically to the PC
>speaker.
>
>  -machine ...,pcspk-audiodev=<name>
>
>
>If we assume that's the desired pattern, then every machine which has
>a built-in audio device should gain some  "{$device}-audiodev" proprerty
>where '$device' is some reasonable name for the built-in audio device
>of the machine.  This would work better if a machine ended up with
>two built-in audio devices and needed separate audiodevs for them.
>

Which is what I wanted to avoid because it creates huge amount of names
which are not easy to find out.  I imagine that the default audio
devices which already exist are not much known and mostly used as-is
without the users needing to figure out what they are.  Especially those
that are enabled even with -nodefaults.  And I expect people who just
want to just emulate an old Palm do not really want to specify the names
of both devices which need an audiodev, especially when one of them is a
touch display or something weird like that.  Moreover you cannot specify
anything for these devices currently, so you cannot use two different
backends now.

I understand that the naming suggests it would be a default audiodev for
anything without one explicitly specified, but I could not think of any
other suitable name.  And I felt like having a default that would be
used for all devices defeats the purpose of removing default audiodev.

If adding a machine option for every such device is the way to go, then
I can try changing that.  Hopefully that would not turn into another
18-patch series =)  So should I prefer that route?

>
>>      /* For compatibility */
>>      object_class_property_add_str(oc, "memory-encryption",
>>          machine_get_memory_encryption, machine_set_memory_encryption);
>> @@ -961,6 +983,7 @@ static void machine_finalize(Object *obj)
>>      g_free(ms->device_memory);
>>      g_free(ms->nvdimms_state);
>>      g_free(ms->numa_state);
>> +    g_free(ms->default_audiodev);
>>  }
>>
>>  bool machine_usb(MachineState *machine)
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index d64b5481e834..5be1de50af03 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -346,6 +346,7 @@ struct MachineState {
>>       */
>>      MemoryRegion *ram;
>>      DeviceMemoryState *device_memory;
>> +    char *default_audiodev;
>>
>>      ram_addr_t ram_size;
>>      ram_addr_t maxram_size;
>> --
>> 2.35.1
>>
>
>With regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>

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