Hi Denis,

On Monday, 4 December 2017 19:40:29 CET Denis Kenzior wrote:
> Hi Szymon,
> 
> <snip>
> 
> > +static struct l_dbus_proxy *l_dbus_proxy_new(struct l_dbus_client
> > *client,
> > +					const char *path, const char *interface)
> 
> I would just call it dbus_proxy_new since it is not exported.  Same with
> l_dbus_proxy_destroy

Done.

> 
> > +{
> > +	struct l_dbus_proxy *proxy;
> > +
> > +	proxy = l_new(struct l_dbus_proxy, 1);
> 
> can condense these to a single line...

OK.

> 
> > +
> > +	proxy->properties_watch = l_dbus_add_signal_watch(client->dbus,
> > +						client->service, path,
> > +						L_DBUS_INTERFACE_PROPERTIES,
> > +						"PropertiesChanged",
> > +						L_DBUS_MATCH_ARGUMENT(0),
> > +						interface, L_DBUS_MATCH_NONE,
> > +						properties_changed_callback,
> > +						proxy);
> > +	if (!proxy->properties_watch) {
> > +		l_free(proxy);
> > +		return NULL;
> > +	}
> > +
> > +	proxy->client = client;
> > +	proxy->interface = l_strdup(interface);
> > +	proxy->path = l_strdup(path);
> > +	proxy->properties = l_queue_new();
> > +
> > +	l_queue_push_tail(client->proxies, proxy);
> > +
> > +	return proxy;
> > +}
> > +
> > +static bool is_ignorable(const char *interface)
> > +{
> > +	static const struct {
> > +		const char *interface;
> > +	} interfaces_to_ignore[] = {
> > +		{ L_DBUS_INTERFACE_OBJECT_MANAGER },
> > +		{ L_DBUS_INTERFACE_INTROSPECTABLE },
> > +		{ L_DBUS_INTERFACE_PROPERTIES },
> > +	};
> > +	size_t i;
> > +
> > +	for (i = 0; i < L_ARRAY_SIZE(interfaces_to_ignore); i++)
> > +		if (!strcmp(interfaces_to_ignore[i].interface, interface))
> > +			return true;
> > +
> > +	return false;
> > +}
> > +
> > +static struct l_dbus_proxy *find_proxy(struct l_dbus_client *client,
> > +					const char *path, const char *interface)
> > +{
> > +	const struct l_queue_entry *entry;
> > +
> > +	for (entry = l_queue_get_entries(client->proxies); entry;
> > +							entry = entry->next) {
> > +		struct l_dbus_proxy *proxy = entry->data;
> > +
> > +		if (!strcmp(proxy->interface, interface) &&
> > +						!strcmp(proxy->path, path))
> > +			return proxy;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static void parse_interface(struct l_dbus_client *client, const char
> > *path, +					const char *interface,
> > +					struct l_dbus_message_iter *properties)
> > +{
> > +	struct l_dbus_proxy *proxy;
> > +
> > +	if (is_ignorable(interface))
> > +		return;
> > +
> > +	proxy = find_proxy(client, path, interface);
> > +	if (!proxy) {
> > +		proxy = l_dbus_proxy_new(client, path, interface);
> > +	}
> 
> No need for {}

Fixed.

> 
> > +
> > +	if (!proxy)
> > +		return;
> > +
> > +	proxy_update_properties(proxy, properties);
> > +
> > +	if (!proxy->ready) {
> > +		proxy->ready = true;
> > +
> > +		if (client->proxy_added_cb)
> > +			client->proxy_added_cb(proxy, client->proxy_cb_data);
> > +	}
> > +}
> > +
> 
> <snip>
> 
> > +
> > +static void service_appeared_callback(struct l_dbus *dbus, void
> > *user_data) +{
> > +	struct l_dbus_client *client = user_data;
> > +
> > +	/* TODO should we allow to set different root? */
> > +	client->objects_call = l_dbus_method_call(dbus, client->service, "/",
> > +						L_DBUS_INTERFACE_OBJECT_MANAGER,
> > +						"GetManagedObjects", NULL,
> > +						get_managed_objects_reply,
> > +						client, NULL);
> > +
> > +	if (client->connect_cb)
> > +		client->connect_cb(client->dbus, client->connect_cb_data);
> > +}
> > +
> > +static void service_disappeared_callback(struct l_dbus *dbus, void
> > *user_data) +{
> > +	struct l_dbus_client *client = user_data;
> > +
> > +	if (client->disconnect_cb)
> > +		client->disconnect_cb(client->dbus, client->disconnect_cb_data);
> > +
> > +	l_queue_clear(client->proxies,
> > +				(l_queue_destroy_func_t)l_dbus_proxy_destroy);
> > +}
> > +
> > +LIB_EXPORT struct l_dbus_client *l_dbus_client_new(struct l_dbus *dbus,
> > +					const char *service, const char *path)
> > +{
> > +	struct l_dbus_client *client;
> > +
> > +	client = l_new(struct l_dbus_client, 1);
> > +
> 
> Can condense these to a single line

OK.

> 
> > +	client->dbus = dbus;
> > +
> > +	client->watch = l_dbus_add_service_watch(dbus, service,
> > +						service_appeared_callback,
> > +						service_disappeared_callback,
> > +						client, NULL);
> > +
> > +	if (!client->watch) {
> > +		l_free(client);
> > +		return NULL;
> > +	}
> > +
> > +	client->service = l_strdup(service);
> > +	client->path = l_strdup(path);
> 
> What is the purpose of this member actually?  We're not using it for the
> ObjectManager root, nor are we using it for anything else.  I also don't
> get its purpose in gdbus either.  Can we simply drop it?

Yeap, I think I initially had getter for it that turned out to be useless but 
forgot to remove member from client. Fixed.

> 
> > +	client->proxies = l_queue_new();
> > +
> > +	return client;
> > +}
> > +
> > +LIB_EXPORT void l_dbus_client_destroy(struct l_dbus_client *client)
> > +{
> > +	if (unlikely(!client))
> > +		return;
> > +
> > +	if (client->watch)
> > +		l_dbus_remove_signal_watch(client->dbus, client->watch);
> > +
> > +	if (client->added_watch)
> > +		l_dbus_remove_signal_watch(client->dbus, client->added_watch);
> > +
> > +	if (client->removed_watch)
> > +		l_dbus_remove_signal_watch(client->dbus, client->removed_watch);
> > +
> > +	if (client->connect_cb_data_destroy)
> > +		client->connect_cb_data_destroy(client->connect_cb_data);
> > +
> > +	if (client->disconnect_cb_data_destroy)
> > +		client->disconnect_cb_data_destroy(client->disconnect_cb_data);
> > +
> > +	if (client->ready_cb_data_destroy)
> > +		client->ready_cb_data_destroy(client->ready_cb_data);
> > +
> > +	if (client->proxy_cb_data_destroy)
> > +		client->proxy_cb_data_destroy(client->proxy_cb_data);
> > +
> > +	if (client->objects_call)
> > +		l_dbus_cancel(client->dbus, client->objects_call)
> > +;
> > +	l_queue_destroy(client->proxies,
> > +				(l_queue_destroy_func_t)l_dbus_proxy_destroy);
> > +
> > +	l_free(client->service);
> > +	l_free(client->path);
> > +	l_free(client);
> > +}
> > +
> > +LIB_EXPORT bool l_dbus_client_set_connect_handler(struct l_dbus_client
> > *client, +						l_dbus_watch_func_t function,
> > +						void *user_data,
> > +						l_dbus_destroy_func_t destroy)
> > +{
> > +	if (unlikely(!client))
> > +		return false;
> > +
> > +	client->connect_cb = function;
> > +	client->connect_cb_data = user_data;
> > +	client->connect_cb_data_destroy = destroy;
> 
> No biggie, but maybe we should call destroy function here in case it is
> already set.

OK.

> 
> > +
> > +	return true;
> > +}
> > +
> > +LIB_EXPORT bool l_dbus_client_set_disconnect_handler(struct l_dbus_client
> > *client, +						l_dbus_watch_func_t function,
> > +						void *user_data,
> > +						l_dbus_destroy_func_t destroy)
> > +{
> > +	if (unlikely(!client))
> > +		return false;
> > +
> > +	client->disconnect_cb = function;
> > +	client->disconnect_cb_data = user_data;
> > +	client->disconnect_cb_data_destroy = destroy;
> 
> as above
> 
> > +
> > +	return true;
> > +}
> > +
> > +LIB_EXPORT bool l_dbus_client_set_ready_handler(struct l_dbus_client
> > *client, +					l_dbus_client_ready_func_t function,
> > +					void *user_data,
> > +					l_dbus_destroy_func_t destroy)
> > +{
> > +	if (unlikely(!client))
> > +		return false;
> > +
> > +	client->ready_cb = function;
> > +	client->ready_cb_data = user_data;
> > +	client->ready_cb_data_destroy = destroy;
> 
> as above
> 
> > +
> > +	return true;
> > +}
> > +
> > +LIB_EXPORT bool l_dbus_client_set_proxy_handlers(struct l_dbus_client
> > *client, +			l_dbus_client_proxy_func_t proxy_added,
> > +			l_dbus_client_proxy_func_t proxy_removed,
> > +			l_dbus_client_property_function_t property_changed,
> > +			void *user_data, l_dbus_destroy_func_t destroy)
> > +{
> > +	if (unlikely(!client))
> > +		return false;
> > +
> > +	client->proxy_added_cb = proxy_added;
> > +	client->proxy_removed_cb = proxy_removed;
> > +	client->properties_changed_cb = property_changed;
> > +	client->proxy_cb_data = user_data;
> > +	client->proxy_cb_data_destroy = destroy;
> 
> as above
> 
> > +
> > +	return true;
> > +}
> 
> <snip>
> 
> Regards,
> -Denis


-- 
pozdrawiam
Szymon Janc