[pulseaudio-discuss] [PATCH 2/7] card: add pa_card_profile.ports

[Tanu Kaskinen]

On Mon, 2015-10-26 at 10:45 +0100, David Henningsson wrote:
> 
> 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?

Yes, they contain the same information, which is the case with any
bidirectional link between two objects.

> 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.

Good catch. When freeing a port or profile, the link needs to be
removed from both ends.

>   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 think it's best to set the both ends of the bidirectional link at the
same time. It's true that the backend probably shouldn't have to spend
two statements to set up the link, though. pa_profile_add_port() could
add the profile to port->profiles, or there could be
pa_card_add_profile_port_link() that sets up both ends of the link. I
think I prefer the latter.

> 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 */
> }

Well, my taste says "avoid searching when possible". Searching makes
the code harder to follow and is also inefficient.

-- 
Tanu