[pulseaudio-discuss] [PATCH 2/2] alsa: load jack detection module

[Tanu Kaskinen]

On Thu, 2011-05-19 at 11:44 -0500, Jorge Eduardo Candelaria wrote:
> From: Margarita Olaya Cabrera <magi at slimlogic.co.uk>
> 
> This patch adds support for udev detection of sound card jack event devices.
> 
> When a new card is detected, the code now also checks for the presence of any
> associated input device via udev, and report the device to module-alsa-card
> via its jack_id parameter.
> 
> Jack support development was kindly sponsored by Wolfson Microelectronics PLC
> 
> Signed-off-by: Margarita Olaya Cabrera <magi at slimlogic.co.uk>
> Signed-off-by: Jorge Eduardo Candelaria <jedu at slimlogic.co.uk>
> ---
> src/modules/module-udev-detect.c |   68 ++++++++++++++++++++++++++++++++++---
> 1 files changed, 62 insertions(+), 6 deletions(-)
> 
> diff --git a/src/modules/module-udev-detect.c b/src/modules/module-udev-detect.c
> index 63ad195..4321b32 100644
> --- a/src/modules/module-udev-detect.c
> +++ b/src/modules/module-udev-detect.c
> @@ -71,6 +71,8 @@ struct userdata {
> 
>     int inotify_fd;
>     pa_io_event *inotify_io;
> +
> +    char *card_name;

This doesn't really seem to be used for anything.

> };
> 
> static const char* const valid_modargs[] = {
> @@ -163,6 +165,21 @@ static pa_bool_t pcm_is_modem(const char *card_idx, const char *pcm) {
>     return is_modem;
> }
> 
> +static pa_bool_t input_node_belongs_to_device(
> +        struct device *d,
> +        const char *node) {
> +
> +    char *cd;
> +    pa_bool_t b;
> +
> +    cd = pa_sprintf_malloc("/sys%s", d->path);
> +
> +    b = pa_startswith(node, cd);
> +    pa_xfree(cd);
> +
> +    return b;
> +}
> +
> static pa_bool_t is_card_busy(const char *id) {
>     char *card_path = NULL, *pcm_path = NULL, *sub_status = NULL;
>     DIR *card_dir = NULL, *pcm_dir = NULL;
> @@ -352,11 +369,26 @@ static void verify_access(struct userdata *u, struct device *d) {
>     }
> }
> 
> +static const char *jack_get_input_id(const char *path) {
> +    int i;
> +
> +    for( i = 0; i < strlen(path); i++) {

strlen() is called in each iteration - it would be better to save its
return value outside the loop.

> +        if (pa_startswith(path + i, "/input"))
> +            return path + i + 6;
> +    }
> +
> +    return NULL;
> +}
> +
> static void card_changed(struct userdata *u, struct udev_device *dev) {
>     struct device *d;
> +    struct udev_enumerate *enumerate;
> +    struct udev_list_entry *item, *first;
>     const char *path;
>     const char *t;
>     char *n;
> +    char *jack_path;
> +    char *jack_input;
> 
>     pa_assert(u);
>     pa_assert(dev);
> @@ -383,6 +415,28 @@ static void card_changed(struct userdata *u, struct udev_device *dev) {
> 
>     n = pa_namereg_make_valid_name(t);
>     d->card_name = pa_sprintf_malloc("alsa_card.%s", n);
> +
> +    if (!(enumerate = udev_enumerate_new(u->udev)))
> +        pa_log("Failed to initialize udev enumerator");
> +    else {
> +        if (!udev_enumerate_add_match_subsystem(enumerate, "input"))
> +            pa_log_debug("Failed to match to subsystem input");
> +
> +        if (udev_enumerate_scan_devices(enumerate))
> +            pa_log_debug("Failed to scan for devices");

Should these two errors abort the enumeration, or is a debug message
really good enough error handling here?

> +
> +        first = udev_enumerate_get_list_entry(enumerate);
> +        udev_list_entry_foreach(item, first) {
> +            jack_path = udev_list_entry_get_name(item);
> +            if (input_node_belongs_to_device(d, jack_path)) {
> +                jack_input = pa_xstrdup(jack_get_input_id(jack_path));
> +                pa_log_debug("jack is %s\n", jack_input);

jack_input can, at least in theory, be null (or if it can't, you should
add an assertion to jack_get_input_id() instead of returning NULL). I'm
not sure what happens if you pass a null pointer as a string to the log
functions - does it crash, or does it work? There's the pa_strnull()
function that returns the string "(null)" if the argument is null, and
the function is being used in similar contexts as this one. Either you
should call pa_strnull() here, or the other uses of pa_strnull() are
redundant.

> +                break;
> +            }
> +        }
> +        udev_enumerate_unref(enumerate);
> +    }
> +
>     d->args = pa_sprintf_malloc("device_id=\"%s\" "
>                                 "name=\"%s\" "
>                                 "card_name=\"%s\" "
> @@ -390,15 +444,19 @@ static void card_changed(struct userdata *u, struct udev_device *dev) {
>                                 "tsched=%s "
>                                 "ignore_dB=%s "
>                                 "sync_volume=%s "
> +                                "jack_id=\"%s\" "
>                                 "card_properties=\"module-udev-detect.discovered=1\"",
>                                 path_get_card_id(path),
>                                 n,
>                                 d->card_name,
>                                 pa_yes_no(u->use_tsched),
>                                 pa_yes_no(u->ignore_dB),
> -                                pa_yes_no(u->sync_volume));
> -    pa_xfree(n);
> +                                pa_yes_no(u->sync_volume),
> +                                jack_input);

The comment about pa_strnull() applies here too.

> 
> +    pa_xfree(n);
> +    pa_xfree(jack_input);
> +    u->card_name = d->card_name;
>     pa_hashmap_put(u->devices, d->path, d);
> 
>     verify_access(u, d);
> @@ -481,13 +539,11 @@ static void monitor_cb(
>         goto fail;
>     }
> 
> -    if (!path_get_card_id(udev_device_get_devpath(dev))) {
> +    if (path_get_card_id(udev_device_get_devpath(dev))) {
> +        process_device(u, dev);
>         udev_device_unref(dev);
> -        return;
>     }
> 
> -    process_device(u, dev);
> -    udev_device_unref(dev);

If path_get_card_id() returns zero, dev doesn't get unreffed. Is this
intentional - did you fix a bug, or introduce one?

>     return;
> 
> fail:


-- 
Tanu