[pulseaudio-discuss] [PATCH 05/17] bluetooth: Parse HandsfreeAudioCard properties

[Tanu Kaskinen]

On Tue, 2014-09-02 at 12:51 +0300, Luiz Augusto von Dentz wrote:
> >> +static hf_audio_card *hf_audio_card_new(pa_bluetooth_backend *hf, const char *path) {
> >> +    hf_audio_card *card = pa_xnew0(hf_audio_card, 1);
> >> +
> >> +    card->path = pa_xstrdup(path);
> >> +    card->hf = hf;
> >> +    card->fd = -1;
> >> +
> >> +    return card;
> >> +}
> >> +
> >> +static void hf_audio_card_free(void *data) {
> >
> > Don't use void pointer here just to avoid a cast in the
> > pa_hashmap_new_full() call, it goes against the convention.
> 
> So we cast when using pa_hashmap_new_full?

Yes.

> >> +            goto fail;
> >> +        }
> >> +
> >> +        dbus_message_iter_get_basic(&value_i, &value);
> >> +
> >> +        if (pa_streq(key, "RemoteAddress"))
> >> +            card->remote = pa_xstrdup(value);
> >> +        else if (pa_streq(key, "LocalAddress"))
> >> +            card->local = pa_xstrdup(value);
> >
> > You should prepare for duplicate properties. The current code leaks
> > memory in case of duplicates.
> 
> I guess you meant to be safe it is better to free before assigning
> anything, perhaps if we handle this together with PropertiesChanged it
> would make these checks common.

Yes I mean that the previously assigned strings need to be freed before
assigning new ones. As for merging this code with the PropertiesChanged
handler, do as you see best (I didn't bother to check what that would
mean in practice). If you do the merge, remember to distinguish between
constant properties and properties that can change. That is, if there
are properties that should never change, but they change nevertheless,
log an error.

> >> +    backend->discovery = pa_shared_get(c, "bluetooth-discovery");
> >
> > I think it would be better to pass the discovery object as an argument
> > to pa_bluetooth_backend_new().
> 
> So it would be passed along with core? I tried changing to just pass
> the discovery object but core is actually used to access the D-Bus
> connection object.

Yes, I suppose it would be best to pass it along with core. Another
alternative would be to add pa_bluetooth_discovery_get_core().

-- 
Tanu