On pe, 2015-02-13 at 08:01 -0600, Denis Kenzior wrote:
> Hi Jukka,
> 
> <snip>
> 
>  >>> +struct filter_data {
> >>> +	struct l_dbus *dbus;
> >>> +	l_dbus_message_func_t handle_func;
> >>> +	l_dbus_watch_func_t connect_func;
> >>> +	l_dbus_watch_func_t disconnect_func;
> >>> +	char *name;
> >>> +	char *owner;
> >>
> >> What is the exact difference between name and owner?  I don't see a
> >> distinction...
> >
> > name is something like "org.foobar" and owner is ":1.101"
> >
> > User registers watch using a name and dbus-daemon sends its
> > notifications about NameOwnerChanged signals using the owner format so
> > we need to track both formats.
> 
> Yes, but you don't actually handle that anywhere.  So I don't see a 
> distinction in the current code.  If this is TBD, then simply don't 
> include this and mention somewhere that only unique bus names are supported.

Hmm, owner and name are handled differently in the code. For example see
the format_rule() function.

> 
> <snip>
> 
> >>> +	if (arg && data->argument && strcmp(arg, data->argument))
> >>> +		goto out;
> >>
> >> This looks wrong
> >
> > Please elaborate more?
> >
> 
> If arg is NULL, the matching continues.  I do not think that is what you 
> want, but I've not read the DBus spec lately.  This needs a unit test case.
> 
> <snip>
> 
> >>> +LIB_EXPORT unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
> >>> +					const char *name,
> >>> +					l_dbus_watch_func_t connect_func,
> >>> +					l_dbus_watch_func_t disconnect_func,
> >>> +					void *user_data,
> >>> +					l_dbus_destroy_func_t destroy)
> >>> +{
> >>> +	struct filter_data *data;
> >>> +
> >>> +	if (!name)
> >>> +		return 0;
> >>> +
> >>> +	if (connect_func)
> >>> +		return -EOPNOTSUPP;
> >>
> >> Why? You seem to be handling that just fine?
> >
> > No, connect watch needs more work as more code needs to be done. I only
> > needed the disconnect watch for my app so in order to speed things up,
> > left the connect to be done later when such need arises. The connect
> > pointer is in the API so that we do not need to change the API later.
> >
> 
> Okay.  But this is still wrong.  The return value is an unsigned int. 
> You're returning a negative errno.

Yep, clearly a bug.

> 
> Might want to make this into an internal private function for now.

Ok.

> 
> Regards,
> -Denis


Jukka