[pulseaudio-discuss] [PATCH v2 2/9] dbus: Track the default sink and source with hooks

[Tanu Kaskinen]

On Mon, 2013-03-18 at 14:00 +0100, David Henningsson wrote:
> On 02/20/2013 07:23 PM, Tanu Kaskinen wrote:
> > ---
> >   src/modules/dbus/iface-core.c |  173 +++++++++++++++++------------------------
> >   1 file changed, 72 insertions(+), 101 deletions(-)
> >
> > diff --git a/src/modules/dbus/iface-core.c b/src/modules/dbus/iface-core.c
> > index a9716bc..4621726 100644
> > --- a/src/modules/dbus/iface-core.c
> > +++ b/src/modules/dbus/iface-core.c
> > @@ -108,9 +108,8 @@ struct pa_dbusiface_core {
> >       pa_hashmap *modules;
> >       pa_hashmap *clients;
> >
> > -    pa_sink *fallback_sink;
> > -    pa_source *fallback_source;
> > -
> > +    pa_hook_slot *default_sink_changed_slot;
> > +    pa_hook_slot *default_source_changed_slot;
> >       pa_hook_slot *sink_put_slot;
> >       pa_hook_slot *sink_unlink_slot;
> >       pa_hook_slot *source_put_slot;
> > @@ -677,13 +676,12 @@ static void handle_get_fallback_sink(DBusConnection *conn, DBusMessage *msg, voi
> >       pa_assert(msg);
> >       pa_assert(c);
> >
> > -    if (!c->fallback_sink) {
> > -        pa_dbus_send_error(conn, msg, PA_DBUS_ERROR_NO_SUCH_PROPERTY,
> > -                           "There are no sinks, and therefore no fallback sink either.");
> > +    if (!c->core->default_sink) {
> > +        pa_dbus_send_error(conn, msg, PA_DBUS_ERROR_NO_SUCH_PROPERTY, "No fallback sink set.");
> >           return;
> >       }
> 
> Is there a reason for using core->default_sink instead of 
> pa_namereg_get_default_sink?

There's this reason: the property change signals should match what
getting the property would return, at least in my opinion. If we'd call
pa_namereg_get_default_sink() here, then the signals would indicate that
no fallback has been set, but getting the fallback would indicate that
there's some fallback set.

It would definitely be good to have a comment about this here.

> E g, the native protocol (e g pactl stat) uses 
> pa_namereg_get_default_sink instead of the direct pointer.
> 
> As for the other dbus patches (and "core: Add hooks for default device 
> changes"), I looked them briefly through and saw nothing to remark on. 
> With the disclaimer that I don't know the dbus stuff very well, but I'd 
> just say it's a very welcome fix, if it fixes the occasional crashes on 
> device unplug.

OK, if you ack my explanation above, I'll push patches 1-4 (Arun wants
to check patch 5 before I push it). Thanks for the review!

-- 
Tanu