Re: [PATCH 08/13] Introduce machine property "audiodev"

[BALATON Zoltan]

On Thu, 28 Sep 2023, Paolo Bonzini wrote:
> From: Martin Kletzander <mkletzan@redhat.com>
>
> 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>
> [Make the property optional, instead of including it in all machines. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/core/machine.c   | 33 +++++++++++++++++++++++++++++++++
> include/hw/boards.h |  7 +++++++
> 2 files changed, 40 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cb38b8cf4cb..6aa49c8d4f1 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -39,6 +39,7 @@
> #include "hw/virtio/virtio.h"
> #include "hw/virtio/virtio-pci.h"
> #include "hw/virtio/virtio-net.h"
> +#include "audio/audio.h"
>
> GlobalProperty hw_compat_8_1[] = {};
> const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> @@ -686,6 +687,26 @@ bool device_type_is_dynamic_sysbus(MachineClass *mc, const 
> char *type)
>     return allowed;
> }
>
> +static char *machine_get_audiodev(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return g_strdup(ms->audiodev);
> +}
> +
> +static void machine_set_audiodev(Object *obj, const char *value,
> +                                 Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    if (!audio_state_by_name(value, errp)) {
> +        return;
> +    }
> +
> +    g_free(ms->audiodev);
> +    ms->audiodev = g_strdup(value);
> +}
> +
> HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
> {
>     int i;
> @@ -931,6 +952,17 @@ out_free:
>     qapi_free_BootConfiguration(config);
> }
>
> +void machine_add_audiodev_property(MachineClass *mc)
> +{
> +    ObjectClass *oc = OBJECT_CLASS(mc);
> +
> +    object_class_property_add_str(oc, "audiodev",
> +                                  machine_get_audiodev,
> +                                  machine_set_audiodev);
> +    object_class_property_set_description(oc, "audiodev",
> +                                          "Audiodev to use for default machine 
> devices");
> +}
> +
> static void machine_class_init(ObjectClass *oc, void *data)
> {
>     MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1136,6 +1168,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->audiodev);
> }
>
> bool machine_usb(MachineState *machine)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 6c67af196a3..b5153f5f85b 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -24,6 +24,7 @@ OBJECT_DECLARE_TYPE(MachineState, MachineClass, MACHINE)
>
> extern MachineState *current_machine;
>
> +void machine_add_audiodev_property(MachineClass *mc);
> void machine_run_board_init(MachineState *machine, const char *mem_path, Error 
> **errp);
> bool machine_usb(MachineState *machine);
> int machine_phandle_start(MachineState *machine);
> @@ -358,6 +359,12 @@ struct MachineState {
>     MemoryRegion *ram;
>     DeviceMemoryState *device_memory;
>
> +    /*
> +     * Included in MachineState for simplicity, but not supported
> +     * unless machine_add_audiodev_property is called.
> +     */

Maybe this comment could be improved saying something like: Only used by 
machines that have default audio parts in which case machine init should 
call machine_add_audiodev_property().

Regards,
BALATON Zoltan

> +    char *audiodev;
> +
>     ram_addr_t ram_size;
>     ram_addr_t maxram_size;
>     uint64_t   ram_slots;