[pulseaudio-discuss] [RFC][PATCH] CoreAudio: add device detection module

[Lennart Poettering]

On Tue, 03.11.09 14:40, Daniel Mack (daniel at caiaq.de) wrote:

> This is for RFC. Don't know if it makes sense to commit it already as
> the module it instantiates is not yet finished. Anyway, let me know what
> you think.

A few comments:

> +typedef struct ca_device ca_device;
> +
> +struct ca_device {
> +    AudioDeviceID id;
> +    unsigned int  module_index;
> +    PA_LLIST_FIELDS(ca_device);
> +};

In PA we generally don't prefix static variables or file-local
structs, since they are unlikely to cause namespace collisions and
this is a nice way to determine easily if something is exported or not
exported API. But that's just nitpicking.

> +
> +struct userdata {
> +    PA_LLIST_HEAD(ca_device, devices);
> +};
> +
> +static int ca_device_added(struct pa_module *m, AudioDeviceID id) {
> +    char args[32];
> +    pa_module *mod;
> +    struct userdata *u = m->userdata;
> +    struct ca_device *dev;
> +
> +    pa_assert(u);
> +
> +    pa_snprintf(args, sizeof(args), "device_id=%d", id);

Using pa_malloc_sprintf() is recommended. I am actually trying to get
rid of the use of pa_snprintf() in PA entirely and am replacing it
with the malloc version wherever I encounter it and it makes sense
(i.e. outside of RT threads).

> +    if (!mod) {
> +        pa_log_info("Failed to load module %s with arguments '%s'", DEVICE_MODULE_NAME, args);
> +        return -1;
> +    }
> +
> +    dev = pa_xnew0(ca_device, 1);
> +    pa_assert(dev);

No need for this assert. The "x" in "pa_xnew0" should make clear that
this is an aborting allocation anyway. 

> +    num_devices = size / sizeof(AudioDeviceID);
> +    device_id = pa_xnew(AudioDeviceID, num_devices);
> +    pa_assert(device_id);

Same here.

> +    if (AudioHardwareAddPropertyListener(kAudioHardwarePropertyDevices, property_listener_proc, m)) {
> +        pa_log("AudioHardwareAddPropertyListener() failed.");
> +        goto fail;
> +    }

Hmm, how are these event callbacks called? From a background thread?
Or is this hooked up to the event loop in some way? Or is this only
one-time during initialization?


Otherwise looks good.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4