On 2015-10-27 06:41, Tanu Kaskinen wrote:
> 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.
A comment on either side that this is just a mirror of the other hashmap
would be nice to help understanding. (I do appreciate the /* port->name
-> port */ comment though, I think we should have more of those.)
>> 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.
Yes.
>> 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.
Regardless of whether we have this extra hashmap or not, I don't mind
you adding a pa_device_port_link_profile() function.
>> 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.
You could claim that adding extra hashmaps is also inefficient, from a
memory usage perspective. But due to the low amount of ports and
profiles I believe both the memory usage and the performance arguments
are negligible in this case.
As for making the code harder to read, you could potentially wrap it in
a function. That said, please look at my version of patch 5/7 and tell
me if you find it hard to read.
Here's where our tastes seem to differ and I've offered a patch where
the new hashmap is not needed. But I won't block on my opinion here, so
if you feel very strongly about needing this hashmap and want to write
all the maintenance needed to make sure both maps are always consistent,
I can offer to review such a patch.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
