Re: [PATCH 1/8] dbus: Rewrite service/disconnect watch APIs on top of filter API

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Andrew,

On 03/23/2016 09:07 PM, Andrew Zaborowski wrote:
> The two advantages are that this should now work on top of kdbus and
> that the full list of watches is not traversed on every notification.
> ---
>   ell/dbus.c | 220 ++++++++++++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 153 insertions(+), 67 deletions(-)
>
> diff --git a/ell/dbus.c b/ell/dbus.c
> index d976496..6bf5734 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -89,6 +89,10 @@ struct l_dbus {
>   	void *debug_data;
>   	struct _dbus_object_tree *tree;
>   	struct _dbus_filter *filter;
> +	struct l_hashmap *services_watched;
> +	struct service_watch_data *service_watch_head;
> +	unsigned int last_service_watch_id;
> +	struct l_idle *service_watch_remove_work;
>
>   	const struct l_dbus_ops *driver;
>   };
> @@ -123,6 +127,18 @@ struct signal_callback {
>   	void *user_data;
>   };
>
> +struct service_watch_data {
> +	char *bus_name;
> +	l_dbus_destroy_func_t destroy_func;
> +	l_dbus_watch_func_t connect_func;
> +	l_dbus_watch_func_t disconnect_func;
> +	void *user_data;
> +	unsigned int id;
> +	bool pending_get_name_owner;
> +	struct service_watch_data *next;
> +	bool removed;
> +};
> +
>   struct dbus1_filter_data {
>   	struct l_dbus *dbus;
>   	char *sender;
> @@ -363,6 +379,35 @@ static void bus_ready(struct l_dbus *dbus)
>   	l_io_set_write_handler(dbus->io, message_write_handler, dbus, NULL);
>   }
>
> +static void name_owner_notify(struct l_dbus *dbus, const char *name,
> +				const char *old, const char *new)
> +{
> +	struct service_watch_data *watch;
> +	bool connect = old && new && !*old && *new;
> +	bool disconnect = old && new && *old && !*new;

This makes my brain hurt.  I also don't think this works properly.  Half 
the functions call name_owner_notify with empty strings and the other 
half calls it with NULLs.

The disconnect can be expressed much simpler as *new == '\0'.

> +
> +	_dbus_filter_name_owner_notify(dbus->filter, name, new);
> +
> +	watch = l_hashmap_lookup(dbus->services_watched, name);
> +
> +	while (watch && !strcmp(watch->bus_name, name)) {
> +		if (watch->removed)
> +			goto next;
> +
> +		if (watch->pending_get_name_owner) {
> +			watch->pending_get_name_owner = false;
> +			if (new && *new)
> +				watch->connect_func(dbus, watch->user_data);
> +		} else if (connect && watch->connect_func)
> +			watch->connect_func(dbus, watch->user_data);
> +		else if (disconnect && watch->disconnect_func)
> +			watch->disconnect_func(dbus, watch->user_data);
> +
> +next:
> +		watch = watch->next;
> +	}
> +}
> +
>   static void hello_callback(struct l_dbus_message *message, void *user_data)
>   {
>   	struct l_dbus *dbus = user_data;
> @@ -752,7 +797,7 @@ static void get_name_owner_reply_cb(struct l_dbus_message *reply,
>   	if (!l_dbus_message_get_arguments(req->message, "s", &name))
>   		return;
>
> -	_dbus_filter_name_owner_notify(req->dbus->filter, name, owner);
> +	name_owner_notify(req->dbus, name, NULL, owner);

should this use "" instead of NULL?

>   }
>
>   static bool classic_get_name_owner(struct l_dbus *bus, const char *name)
> @@ -800,7 +845,7 @@ static void name_owner_changed_cb(struct l_dbus_message *message,
>   	if (!l_dbus_message_get_arguments(message, "sss", &name, &old, &new))
>   		return;
>
> -	_dbus_filter_name_owner_notify(dbus->filter, name, new);
> +	name_owner_notify(dbus, name, old, new);
>   }
>
>   static struct l_dbus *setup_dbus1(int fd, const char *guid)
> @@ -1000,15 +1045,22 @@ static void kdbus_name_owner_change_func(const char *name, uint64_t old_owner,
>   						void *user_data)
>   {
>   	struct kdbus_message_recv_data *recv_data = user_data;
> -	char owner[32];
> +	char old[32], new[32];
>
>   	l_util_debug(recv_data->dbus->debug_handler,
>   			recv_data->dbus->debug_data,
>   			"Read KDBUS Name Owner Change notification");
>
> -	snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner);
> +	if (old_owner)
> +		snprintf(old, sizeof(old), ":1.%" PRIu64, new_owner);

old_owner here, not new_owner?

> +	else
> +		*old = '\0';
> +	if (new_owner)
> +		snprintf(new, sizeof(new), ":1.%" PRIu64, new_owner);
> +	else
> +		*new = '\0';
>
> -	_dbus_filter_name_owner_notify(recv_data->dbus->filter, name, owner);
> +	name_owner_notify(recv_data->dbus, name, old, new);
>   }
>
>   static struct l_dbus_message *kdbus_recv_message(struct l_dbus *dbus)
> @@ -1070,15 +1122,13 @@ static bool kdbus_get_name_owner(struct l_dbus *dbus, const char *name)
>   	char owner[32];
>
>   	owner_id = _dbus_kernel_get_name_owner(fd, kdbus->kdbus_pool, name);
> -	if (!owner_id) {
> -		l_util_debug(dbus->debug_handler,
> -				dbus->debug_data, "Error getting name owner");
> -		return false;
> -	}
>
> -	snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
> +	if (owner_id)
> +		snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id);
> +	else
> +		*owner = '\0';
>
> -	_dbus_filter_name_owner_notify(dbus->filter, name, owner);
> +	name_owner_notify(dbus, name, NULL, owner);

"" instead of NULL?

>
>   	return true;
>   }
> @@ -1222,6 +1272,17 @@ LIB_EXPORT struct l_dbus *l_dbus_new_default(enum l_dbus_bus bus)
>   	return setup_address(address);
>   }
>
> +static void service_watch_data_free(void *data)
> +{
> +	struct service_watch_data *watch = data;
> +
> +	if (watch->destroy_func)
> +		watch->destroy_func(watch->user_data);
> +
> +	l_free(watch->bus_name);
> +	l_free(watch);
> +}
> +
>   LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
>   {
>   	if (unlikely(!dbus))
> @@ -1236,6 +1297,11 @@ LIB_EXPORT void l_dbus_destroy(struct l_dbus *dbus)
>   	l_hashmap_destroy(dbus->message_list, message_list_destroy);
>   	l_queue_destroy(dbus->message_queue, message_queue_destroy);
>
> +	l_hashmap_destroy(dbus->services_watched, service_watch_data_free);
> +
> +	if (dbus->service_watch_remove_work)
> +		l_idle_remove(dbus->service_watch_remove_work);
> +
>   	l_io_destroy(dbus->io);
>
>   	if (dbus->disconnect_destroy)
> @@ -1809,33 +1875,6 @@ void _dbus1_name_owner_changed_filter(struct l_dbus_message *message,
>   	}
>   }
>
> -static void _dbus1_get_name_owner_reply(struct l_dbus_message *message,
> -							void *user_data)
> -{
> -	struct dbus1_filter_data *data = user_data;
> -	const char *name;
> -
> -	data->get_name_owner_id = 0;
> -
> -	/* No name owner yet */
> -	if (l_dbus_message_is_error(message))
> -		return;
> -
> -	/* Shouldn't happen */
> -	if (!l_dbus_message_get_arguments(message, "s", &name))
> -		return;
> -
> -	/* If the signal arrived before the reply, don't do anything */
> -	if (data->owner && !strcmp(data->owner, name))
> -		return;
> -
> -	l_free(data->owner);
> -	data->owner = l_strdup(name);
> -
> -	if (data->connect_func)
> -		data->connect_func(data->dbus, data->user_data);
> -}
> -
>   LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
>   					const char *name,
>   					l_dbus_watch_func_t disconnect_func,
> @@ -1846,56 +1885,103 @@ LIB_EXPORT unsigned int l_dbus_add_disconnect_watch(struct l_dbus *dbus,
>   					user_data, destroy);
>   }
>
> -unsigned int l_dbus_add_service_watch(struct l_dbus *dbus,
> +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 dbus1_filter_data *data;
> +	struct service_watch_data *watch, *first;
>
>   	if (!name)
>   		return 0;
>
> -	data = _dbus1_filter_data_get(dbus, _dbus1_name_owner_changed_filter,
> -				DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
> -				L_DBUS_INTERFACE_DBUS, "NameOwnerChanged",
> -				name,
> -				disconnect_func,
> -				user_data,
> -				destroy);
> -	if (!data)
> -		return 0;
> +	if (!dbus->services_watched)
> +		dbus->services_watched = l_hashmap_string_new();
> +
> +	watch = l_new(struct service_watch_data, 1);
> +	watch->bus_name = l_strdup(name);
> +	watch->id = ++dbus->last_service_watch_id;
> +	watch->connect_func = connect_func;
> +	watch->disconnect_func = disconnect_func;
> +	watch->destroy_func = destroy;
> +
> +	/*
> +	 * Make sure that all the entries for the same bus name are
> +	 * grouped together on the list and that the first entry for the
> +	 * name is in the hashmap for easy lookups.
> +	 */
> +	first = l_hashmap_lookup(dbus->services_watched, name);
> +	if (first) {
> +		watch->next = first->next;
> +		first->next = watch;
> +	} else {
> +		l_hashmap_insert(dbus->services_watched, name, watch);
>
> -	add_match(data);
> +		watch->next = dbus->service_watch_head;
> +		dbus->service_watch_head = watch;
> +	}
>
>   	if (connect_func) {
> -		struct l_dbus_message *message;
> +		watch->pending_get_name_owner = true;
>
> -		data->connect_func = connect_func;
> +		dbus->driver->filter_ops.get_name_owner(dbus, name);
> +	}
>
> -		message = l_dbus_message_new_method_call(dbus,
> -							DBUS_SERVICE_DBUS,
> -							DBUS_PATH_DBUS,
> -							L_DBUS_INTERFACE_DBUS,
> -							"GetNameOwner");
> +	return watch->id;
> +}
>
> -		l_dbus_message_set_arguments(message, "s", name);
> +static void service_watch_remove(struct l_idle *idle, void *user_data)
> +{
> +	struct l_dbus *dbus = user_data;
> +	struct service_watch_data **watch, *tmp;
>
> -		data->get_name_owner_id =
> -			l_dbus_send_with_reply(dbus, message,
> -						_dbus1_get_name_owner_reply,
> -						data, NULL);
> -	}
> +	l_idle_remove(dbus->service_watch_remove_work);
> +	dbus->service_watch_remove_work = NULL;
> +
> +	for (watch = &dbus->service_watch_head; *watch;
> +			watch = &(*watch)->next) {
> +		if (!(*watch)->removed)
> +			continue;
> +
> +		tmp = *watch;
> +		*watch = tmp->next;
> +
> +		/* Check if this was the first entry for the bus name */
> +		if (l_hashmap_lookup(dbus->services_watched, tmp->bus_name) ==
> +				tmp) {
> +			l_hashmap_remove(dbus->services_watched, tmp->bus_name);
> +
> +			if (tmp->next && !strcmp(tmp->bus_name,
> +						tmp->next->bus_name))
> +				l_hashmap_insert(dbus->services_watched,
> +						tmp->bus_name, tmp->next);

I really wonder if we should just add l_hashmap_find or something here 
and avoid the linked-list on the side.  It feels a bit kludgy to me. 
Alternatively, let the user remove the service watch by name instead of 
messing with ids.

> +		}
>
> -	return l_dbus_register(dbus, _dbus1_signal_dispatcher, data,
> -							filter_data_destroy);
> +		service_watch_data_free(tmp);
> +	}
>   }
>
>   LIB_EXPORT bool l_dbus_remove_watch(struct l_dbus *dbus, unsigned int id)
>   {
> -	return l_dbus_unregister(dbus, id);
> +	struct service_watch_data *watch;
> +
> +	for (watch = dbus->service_watch_head; watch; watch = watch->next)
> +		if (watch->id == id && !watch->removed)
> +			break;
> +
> +	if (!watch)
> +		return false;
> +
> +	watch->removed = true;

Do you also want to make sure that the callbacks are not called by 
setting them to NULL or something?  Otherwise you can remove a watch, 
and it will still be called due to pending events...

> +
> +	if (!dbus->service_watch_remove_work)
> +		dbus->service_watch_remove_work = l_idle_create(
> +							service_watch_remove,
> +							dbus, NULL);
> +
> +	return true;
>   }
>
>   /**
>

Regards,
-Denis