Re: [PATCH v2 2/3] dbus: Add disconnect watch support

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

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.

<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.

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

Regards,
-Denis