On 2015-10-23 12:56, Tanu Kaskinen wrote:
> Having ports accessible from pa_card_profile allows checking whether all ports
> of a profile are unavailable, and therefore helps with managing the profile
> availability (implemented in a later patch).
Hmm.
So now we have ports referencing profiles and profiles referencing
ports, and they essentially contain the same information?
So, I have two major concerns about this patch:
1) Potentially dangling pointers. Ports are freed before profiles (due
to the existing ports->profiles array, or it just happened to be that
way). So when profile->ports is freed, its keys are dangling pointers,
and from a quick glance it looks like remove_entry() (called from
pa_hashmap_free) could potentially call the hash function for a now
invalid key.
2) If we're making it mandatory (or at least useful) to fill this
array in, why don't we do it in pa_card_new instead of in every backend?
I'd say it's probably easier just to calculate the array every time you
need it. It's not hard to do:
for each port in profile->card->ports {
if !pa_hashmap_get(port->profiles, profile->name)
continue;
/* do stuff here */
}
Am I missing something?
> ---
> src/modules/alsa/alsa-mixer.c | 4 +++-
> src/modules/alsa/alsa-ucm.c | 1 +
> src/modules/bluetooth/module-bluez4-device.c | 6 ++++++
> src/modules/bluetooth/module-bluez5-device.c | 6 ++++++
> src/pulsecore/card.c | 8 ++++++++
> src/pulsecore/card.h | 18 ++++++++++++------
> 6 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
> index 9e06ba4..5289a12 100644
> --- a/src/modules/alsa/alsa-mixer.c
> +++ b/src/modules/alsa/alsa-mixer.c
> @@ -4768,8 +4768,10 @@ static pa_device_port* device_port_alsa_init(pa_hashmap *ports, /* card ports */
> path->port = p;
> }
>
> - if (cp)
> + if (cp) {
> pa_hashmap_put(p->profiles, cp->name, cp);
> + pa_card_profile_add_port(cp, p);
> + }
>
> if (extra) {
> pa_hashmap_put(extra, p->name, p);
> diff --git a/src/modules/alsa/alsa-ucm.c b/src/modules/alsa/alsa-ucm.c
> index 42f3242..68fcc26 100644
> --- a/src/modules/alsa/alsa-ucm.c
> +++ b/src/modules/alsa/alsa-ucm.c
> @@ -791,6 +791,7 @@ static void ucm_add_port_combination(
> if (cp) {
> pa_log_debug("Adding profile %s to port %s.", cp->name, port->name);
> pa_hashmap_put(port->profiles, cp->name, cp);
> + pa_card_profile_add_port(cp, port);
> }
>
> if (hash) {
> diff --git a/src/modules/bluetooth/module-bluez4-device.c b/src/modules/bluetooth/module-bluez4-device.c
> index a23c2a9..af36e6a 100644
> --- a/src/modules/bluetooth/module-bluez4-device.c
> +++ b/src/modules/bluetooth/module-bluez4-device.c
> @@ -2180,6 +2180,7 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
> p->max_sink_channels = 2;
> p->max_source_channels = 0;
> pa_hashmap_put(output_port->profiles, p->name, p);
> + pa_card_profile_add_port(p, output_port);
>
> d = PA_CARD_PROFILE_DATA(p);
> *d = PA_BLUEZ4_PROFILE_A2DP;
> @@ -2191,6 +2192,7 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
> p->max_sink_channels = 0;
> p->max_source_channels = 2;
> pa_hashmap_put(input_port->profiles, p->name, p);
> + pa_card_profile_add_port(p, input_port);
>
> d = PA_CARD_PROFILE_DATA(p);
> *d = PA_BLUEZ4_PROFILE_A2DP_SOURCE;
> @@ -2203,6 +2205,8 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
> p->max_source_channels = 1;
> pa_hashmap_put(input_port->profiles, p->name, p);
> pa_hashmap_put(output_port->profiles, p->name, p);
> + pa_card_profile_add_port(p, input_port);
> + pa_card_profile_add_port(p, output_port);
>
> d = PA_CARD_PROFILE_DATA(p);
> *d = PA_BLUEZ4_PROFILE_HSP;
> @@ -2215,6 +2219,8 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
> p->max_source_channels = 1;
> pa_hashmap_put(input_port->profiles, p->name, p);
> pa_hashmap_put(output_port->profiles, p->name, p);
> + pa_card_profile_add_port(p, input_port);
> + pa_card_profile_add_port(p, output_port);
>
> d = PA_CARD_PROFILE_DATA(p);
> *d = PA_BLUEZ4_PROFILE_HFGW;
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 6ebcda2..19ce2b7 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -1790,6 +1790,7 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
> cp->max_sink_channels = 2;
> cp->max_source_channels = 0;
> pa_hashmap_put(output_port->profiles, cp->name, cp);
> + pa_card_profile_add_port(cp, output_port);
>
> p = PA_CARD_PROFILE_DATA(cp);
> *p = PA_BLUETOOTH_PROFILE_A2DP_SINK;
> @@ -1801,6 +1802,7 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
> cp->max_sink_channels = 0;
> cp->max_source_channels = 2;
> pa_hashmap_put(input_port->profiles, cp->name, cp);
> + pa_card_profile_add_port(cp, input_port);
>
> p = PA_CARD_PROFILE_DATA(cp);
> *p = PA_BLUETOOTH_PROFILE_A2DP_SOURCE;
> @@ -1813,6 +1815,8 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
> cp->max_source_channels = 1;
> pa_hashmap_put(input_port->profiles, cp->name, cp);
> pa_hashmap_put(output_port->profiles, cp->name, cp);
> + pa_card_profile_add_port(cp, input_port);
> + pa_card_profile_add_port(cp, output_port);
>
> p = PA_CARD_PROFILE_DATA(cp);
> *p = PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT;
> @@ -1825,6 +1829,8 @@ static pa_card_profile *create_card_profile(struct userdata *u, const char *uuid
> cp->max_source_channels = 1;
> pa_hashmap_put(input_port->profiles, cp->name, cp);
> pa_hashmap_put(output_port->profiles, cp->name, cp);
> + pa_card_profile_add_port(cp, input_port);
> + pa_card_profile_add_port(cp, output_port);
>
> p = PA_CARD_PROFILE_DATA(cp);
> *p = PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY;
> diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
> index c8b97b7..4ef7900 100644
> --- a/src/pulsecore/card.c
> +++ b/src/pulsecore/card.c
> @@ -45,6 +45,7 @@ pa_card_profile *pa_card_profile_new(const char *name, const char *description,
> c->name = pa_xstrdup(name);
> c->description = pa_xstrdup(description);
> c->available = PA_AVAILABLE_UNKNOWN;
> + c->ports = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>
> return c;
> }
> @@ -52,11 +53,18 @@ pa_card_profile *pa_card_profile_new(const char *name, const char *description,
> void pa_card_profile_free(pa_card_profile *c) {
> pa_assert(c);
>
> + pa_hashmap_free(c->ports);
> pa_xfree(c->name);
> pa_xfree(c->description);
> pa_xfree(c);
> }
>
> +void pa_card_profile_add_port(pa_card_profile *profile, pa_device_port *port) {
> + pa_assert(profile);
> +
> + pa_hashmap_put(profile->ports, port->name, port);
> +}
> +
> void pa_card_profile_set_available(pa_card_profile *c, pa_available_t available) {
> pa_core *core;
>
> diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
> index 3e2c004..1c33958 100644
> --- a/src/pulsecore/card.h
> +++ b/src/pulsecore/card.h
> @@ -22,19 +22,21 @@
>
> typedef struct pa_card pa_card;
>
> -#include <pulse/proplist.h>
> -#include <pulsecore/core.h>
> -#include <pulsecore/module.h>
> -#include <pulsecore/idxset.h>
> -
> /* This enum replaces pa_port_available_t (defined in pulse/def.h) for
> - * internal use, so make sure both enum types stay in sync. */
> + * internal use, so make sure both enum types stay in sync. This is defined
> + * before the #includes, because device-port.h depends on this enum. */
> typedef enum pa_available {
> PA_AVAILABLE_UNKNOWN = 0,
> PA_AVAILABLE_NO = 1,
> PA_AVAILABLE_YES = 2,
> } pa_available_t;
>
> +#include <pulse/proplist.h>
> +#include <pulsecore/core.h>
> +#include <pulsecore/device-port.h>
> +#include <pulsecore/module.h>
> +#include <pulsecore/idxset.h>
> +
> typedef struct pa_card_profile {
> pa_card *card;
> char *name;
> @@ -43,6 +45,8 @@ typedef struct pa_card_profile {
> unsigned priority;
> pa_available_t available; /* PA_AVAILABLE_UNKNOWN, PA_AVAILABLE_NO or PA_AVAILABLE_YES */
>
> + pa_hashmap *ports; /* port name -> pa_device_port */
> +
> /* We probably want to have different properties later on here */
> unsigned n_sinks;
> unsigned n_sources;
> @@ -100,6 +104,8 @@ typedef struct pa_card_new_data {
> pa_card_profile *pa_card_profile_new(const char *name, const char *description, size_t extra);
> void pa_card_profile_free(pa_card_profile *c);
>
> +void pa_card_profile_add_port(pa_card_profile *profile, pa_device_port *port);
> +
> /* The profile's available status has changed */
> void pa_card_profile_set_available(pa_card_profile *c, pa_available_t available);
>
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
