On Tue, 2015-10-27 at 08:46 +0100, David Henningsson wrote:
>
> 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.
I'm afraid your patch doesn't work for UCM. You added the profile
status updating to this code:
  for (tp = tports; tp->port; tp++)
        if (tp->avail != PA_AVAILABLE_NO)
           pa_device_port_set_available(tp->port, tp->avail);
    for (tp = tports; tp->port; tp++)
        if (tp->avail == PA_AVAILABLE_NO)
           pa_device_port_set_available(tp->port, tp->avail);
But the for loops don't run when UCM is in use, because tports isn't
populated.
--Â
Tanu
