[PATCH ] tools: Add unregister gatt service

[PATCH ] tools: Add unregister gatt service

[Bharat Panda]

Unregister gatt service on proxy disconnect and removes service
data on Unregister complete.
---
 tools/gatt-service.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/tools/gatt-service.c b/tools/gatt-service.c
index 6bca404..2102230 100644
--- a/tools/gatt-service.c
+++ b/tools/gatt-service.c
@@ -53,6 +53,8 @@ static GMainLoop *main_loop;
 static GSList *services;
 static DBusConnection *connection;
 
+static int id;
+
 struct characteristic {
 	char *uuid;
 	char *path;
@@ -332,10 +334,10 @@ static gboolean register_characteristic(const char *chr_uuid,
 
 static char *register_service(const char *uuid)
 {
-	static int id = 1;
 	char *path;
 
-	path = g_strdup_printf("/service%d", id++);
+	id++;
+	path = g_strdup_printf("/service%d", id);
 	if (!g_dbus_register_interface(connection, path, GATT_SERVICE_IFACE,
 				NULL, NULL, service_properties,
 				g_strdup(uuid), g_free)) {
@@ -373,6 +375,40 @@ static void create_services()
 	printf("Registered service: %s\n", service_path);
 }
 
+static void remove_services()
+{
+	char *service_path = g_strdup_printf("/service%d", id);;
+
+	services = g_slist_remove(services, service_path);
+	g_dbus_unregister_interface(connection, service_path,
+						GATT_SERVICE_IFACE);
+
+	printf("Unregistered: %s\n", service_path);
+	g_free(service_path);
+}
+
+static void unregister_external_service_reply(DBusPendingCall *call,
+							void *user_data)
+{
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+	DBusError derr;
+
+	dbus_error_init(&derr);
+	dbus_set_error_from_message(&derr, reply);
+
+	if (dbus_error_is_set(&derr)) {
+		printf("UnRegisterService: %s\n", derr.message);
+		goto done;
+	}
+
+	printf("UnRegisterService: OK\n");
+	remove_services();
+
+done:
+	dbus_message_unref(reply);
+	dbus_error_free(&derr);
+}
+
 static void register_external_service_reply(DBusPendingCall *call,
 							void *user_data)
 {
@@ -391,6 +427,37 @@ static void register_external_service_reply(DBusPendingCall *call,
 	dbus_error_free(&derr);
 }
 
+static void unregister_external_service(gpointer a, gpointer b)
+{
+	DBusConnection *conn = b;
+	const char *path = a;
+	DBusMessage *msg;
+	DBusPendingCall *call;
+	DBusMessageIter iter;
+
+	msg = dbus_message_new_method_call("org.bluez", "/org/bluez",
+					GATT_MGR_IFACE, "UnregisterService");
+	if (!msg) {
+		printf("Couldn't allocate D-Bus message\n");
+		return;
+	}
+
+	dbus_message_iter_init_append(msg, &iter);
+
+	dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH, &path);
+
+	if (!g_dbus_send_message_with_reply(conn, msg, &call, -1)) {
+		dbus_message_unref(msg);
+		return;
+	}
+
+	dbus_message_unref(msg);
+
+	dbus_pending_call_set_notify(call, unregister_external_service_reply,
+								NULL, NULL);
+	dbus_pending_call_unref(call);
+}
+
 static void register_external_service(gpointer a, gpointer b)
 {
 	DBusConnection *conn = b;
@@ -432,6 +499,11 @@ static void connect_handler(DBusConnection *conn, void *user_data)
 	g_slist_foreach(services, register_external_service, conn);
 }
 
+static void disconnect_handler(DBusConnection *conn, void *user_data)
+{
+	g_slist_foreach(services, unregister_external_service, conn);
+}
+
 static gboolean signal_handler(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
@@ -524,6 +596,7 @@ int main(int argc, char *argv[])
 	client = g_dbus_client_new(connection, "org.bluez", "/org/bluez");
 
 	g_dbus_client_set_connect_watch(client, connect_handler, NULL);
+	g_dbus_client_set_disconnect_watch(client, disconnect_handler, NULL);
 
 	g_main_loop_run(main_loop);
 
-- 
1.9.1

Re: [PATCH ] tools: Add unregister gatt service

[Johan Hedberg]

Hi Bharat,

On Mon, Jul 21, 2014, Bharat Panda wrote:
> +static int id;
> +
>  struct characteristic {
>  	char *uuid;
>  	char *path;
> @@ -332,10 +334,10 @@ static gboolean register_characteristic(const char *chr_uuid,
>  
>  static char *register_service(const char *uuid)
>  {
> -	static int id = 1;

Making id public looks unnecessary to me since after the registration
you've got the path which you can pass to the unregistration procedure.

> +static void remove_services()
> +{
> +	char *service_path = g_strdup_printf("/service%d", id);;
> +
> +	services = g_slist_remove(services, service_path);

This makes even less sense to me. You allocate a new pointer
(service_path) and expect it to magically exist in the services list?
That's not how g_slist_remove works. It knows nothing about strings.
Instead you'd want to get hold of the original path string and remove
the right entry based on that.

> +	if (dbus_error_is_set(&derr)) {
> +		printf("UnRegisterService: %s\n", derr.message);

It's UnregisterService. Not UnRegisterService.

> +	printf("UnRegisterService: OK\n");

Same here.

> +static void unregister_external_service(gpointer a, gpointer b)
> +{
> +	DBusConnection *conn = b;
> +	const char *path = a;
> +	DBusMessage *msg;
> +	DBusPendingCall *call;
> +	DBusMessageIter iter;
> +
> +	msg = dbus_message_new_method_call("org.bluez", "/org/bluez",
> +					GATT_MGR_IFACE, "UnregisterService");
> +	if (!msg) {
> +		printf("Couldn't allocate D-Bus message\n");
> +		return;
> +	}
> +
> +	dbus_message_iter_init_append(msg, &iter);
> +
> +	dbus_message_iter_append_basic(&iter, DBUS_TYPE_OBJECT_PATH, &path);
> +
> +	if (!g_dbus_send_message_with_reply(conn, msg, &call, -1)) {
> +		dbus_message_unref(msg);
> +		return;
> +	}
> +
> +	dbus_message_unref(msg);
> +
> +	dbus_pending_call_set_notify(call, unregister_external_service_reply,
> +								NULL, NULL);
> +	dbus_pending_call_unref(call);
> +}
> +
>  static void register_external_service(gpointer a, gpointer b)
>  {
>  	DBusConnection *conn = b;
> @@ -432,6 +499,11 @@ static void connect_handler(DBusConnection *conn, void *user_data)
>  	g_slist_foreach(services, register_external_service, conn);
>  }
>  
> +static void disconnect_handler(DBusConnection *conn, void *user_data)
> +{
> +	g_slist_foreach(services, unregister_external_service, conn);
> +}
> +
>  static gboolean signal_handler(GIOChannel *channel, GIOCondition cond,
>  							gpointer user_data)
>  {
> @@ -524,6 +596,7 @@ int main(int argc, char *argv[])
>  	client = g_dbus_client_new(connection, "org.bluez", "/org/bluez");
>  
>  	g_dbus_client_set_connect_watch(client, connect_handler, NULL);
> +	g_dbus_client_set_disconnect_watch(client, disconnect_handler, NULL);

When exactly would disconnect_handler be called in practice? At least
one case seems to be if bluetoothd exits in which case it seems quite
wasteful to try to make any method calls to a non-existing service.
What other scenarios would disconnect_handler be called in?

Johan

RE: [PATCH ] tools: Add unregister gatt service

[Bharat Bhusan Panda]

Hi Johan,

> On Mon, Jul 21, 2014, Bharat Panda wrote:
> > +static int id;
> > +
> >  struct characteristic {
> >  	char *uuid;
> >  	char *path;
> > @@ -332,10 +334,10 @@ static gboolean register_characteristic(const
> > char *chr_uuid,
> >
> >  static char *register_service(const char *uuid)  {
> > -	static int id = 1;
> 
> Making id public looks unnecessary to me since after the registration
you've
> got the path which you can pass to the unregistration procedure.

The id was made public, because service path was not public. We need to make
either id or service path as public to unregister interface path.
> 
> > +static void remove_services()
> > +{
> > +	char *service_path = g_strdup_printf("/service%d", id);;
> > +
> > +	services = g_slist_remove(services, service_path);
> 
> This makes even less sense to me. You allocate a new pointer
> (service_path) and expect it to magically exist in the services list?
> That's not how g_slist_remove works. It knows nothing about strings.
> Instead you'd want to get hold of the original path string and remove the
> right entry based on that.

Yes, this is not needed actually, it will be removed in following patch.
> 
> > +	if (dbus_error_is_set(&derr)) {
> > +		printf("UnRegisterService: %s\n", derr.message);
> 
> It's UnregisterService. Not UnRegisterService.
> 
> > +	printf("UnRegisterService: OK\n");
> 
> Same here.
> 
> > +static void unregister_external_service(gpointer a, gpointer b) {
> > +	DBusConnection *conn = b;
> > +	const char *path = a;
> > +	DBusMessage *msg;
> > +	DBusPendingCall *call;
> > +	DBusMessageIter iter;
> > +
> > +	msg = dbus_message_new_method_call("org.bluez", "/org/bluez",
> > +					GATT_MGR_IFACE,
> "UnregisterService");
> > +	if (!msg) {
> > +		printf("Couldn't allocate D-Bus message\n");
> > +		return;
> > +	}
> > +
> > +	dbus_message_iter_init_append(msg, &iter);
> > +
> > +	dbus_message_iter_append_basic(&iter,
> DBUS_TYPE_OBJECT_PATH, &path);
> > +
> > +	if (!g_dbus_send_message_with_reply(conn, msg, &call, -1)) {
> > +		dbus_message_unref(msg);
> > +		return;
> > +	}
> > +
> > +	dbus_message_unref(msg);
> > +
> > +	dbus_pending_call_set_notify(call,
> unregister_external_service_reply,
> > +								NULL, NULL);
> > +	dbus_pending_call_unref(call);
> > +}
> > +
> >  static void register_external_service(gpointer a, gpointer b)  {
> >  	DBusConnection *conn = b;
> > @@ -432,6 +499,11 @@ static void connect_handler(DBusConnection
> *conn, void *user_data)
> >  	g_slist_foreach(services, register_external_service, conn);  }
> >
> > +static void disconnect_handler(DBusConnection *conn, void *user_data)
> > +{
> > +	g_slist_foreach(services, unregister_external_service, conn); }
> > +
> >  static gboolean signal_handler(GIOChannel *channel, GIOCondition cond,
> >  							gpointer user_data)
> >  {
> > @@ -524,6 +596,7 @@ int main(int argc, char *argv[])
> >  	client = g_dbus_client_new(connection, "org.bluez", "/org/bluez");
> >
> >  	g_dbus_client_set_connect_watch(client, connect_handler, NULL);
> > +	g_dbus_client_set_disconnect_watch(client, disconnect_handler,
> > +NULL);
> 
> When exactly would disconnect_handler be called in practice? At least one
> case seems to be if bluetoothd exits in which case it seems quite wasteful
to
> try to make any method calls to a non-existing service.
> What other scenarios would disconnect_handler be called in?

On running the gatt-service, it registers the service and updates gatt db
with the service path.
On exiting the gatt-service, it should call "UnregisterService" and clear
the gatt service db. 
Otherwise, on next run of gatt-service, when it registers the same service
path, 
it gets an error, service already exists. 

Best Regards,
Bharat

Re: [PATCH ] tools: Add unregister gatt service

[Johan Hedberg]

Hi Bharat,

On Mon, Jul 21, 2014, Bharat Bhusan Panda wrote:
> > > +static int id;
> > > +
> > >  struct characteristic {
> > >  	char *uuid;
> > >  	char *path;
> > > @@ -332,10 +334,10 @@ static gboolean register_characteristic(const
> > > char *chr_uuid,
> > >
> > >  static char *register_service(const char *uuid)  {
> > > -	static int id = 1;
> > 
> > Making id public looks unnecessary to me since after the
> > registration you've got the path which you can pass to the
> > unregistration procedure.
> 
> The id was made public, because service path was not public. We need
> to make either id or service path as public to unregister interface
> path.

Can't you just pass it as user_data to unregister_external_service_reply?

> > >  static gboolean signal_handler(GIOChannel *channel, GIOCondition cond,
> > >  							gpointer user_data)
> > >  {
> > > @@ -524,6 +596,7 @@ int main(int argc, char *argv[])
> > >  	client = g_dbus_client_new(connection, "org.bluez", "/org/bluez");
> > >
> > >  	g_dbus_client_set_connect_watch(client, connect_handler, NULL);
> > > +	g_dbus_client_set_disconnect_watch(client, disconnect_handler,
> > > +NULL);
> > 
> > When exactly would disconnect_handler be called in practice? At
> > least one case seems to be if bluetoothd exits in which case it
> > seems quite wasteful to try to make any method calls to a
> > non-existing service. What other scenarios would disconnect_handler
> > be called in?
> 
> On running the gatt-service, it registers the service and updates gatt db
> with the service path.
> On exiting the gatt-service, it should call "UnregisterService" and clear
> the gatt service db. 

How exactly does gatt-service exit? If it stops iterating the main loop
your D-Bus reply callback would never get called and most of the new
code you're adding would never get run.

> Otherwise, on next run of gatt-service, when it registers the same service
> path, it gets an error, service already exists. 

bluetoothd should monitor all of its clients and clean up after them if
they exit without unregistering. If that's not happening then it's a bug
that's much more important to fix than what you're trying to do here. A
quick look at src/gatt-dbus.c shows that it's at least trying to clean
up, but maybe there's a bug somewhere there.

Johan

RE: [PATCH ] tools: Add unregister gatt service

[Bharat Bhusan Panda]

Hi Johan,

> > > > +static int id;
> > > > +
> > > >  struct characteristic {
> > > >  	char *uuid;
> > > >  	char *path;
> > > > @@ -332,10 +334,10 @@ static gboolean
> > > > register_characteristic(const char *chr_uuid,
> > > >
> > > >  static char *register_service(const char *uuid)  {
> > > > -	static int id = 1;
> > >
> > > Making id public looks unnecessary to me since after the
> > > registration you've got the path which you can pass to the
> > > unregistration procedure.
> >
> > The id was made public, because service path was not public. We need
> > to make either id or service path as public to unregister interface
> > path.
> 
> Can't you just pass it as user_data to unregister_external_service_reply?

Yes, I will modify it accordingly.
> 
> > > >  static gboolean signal_handler(GIOChannel *channel, GIOCondition
> cond,
> > > >  							gpointer
user_data)
> > > >  {
> > > > @@ -524,6 +596,7 @@ int main(int argc, char *argv[])
> > > >  	client = g_dbus_client_new(connection, "org.bluez",
> > > > "/org/bluez");
> > > >
> > > >  	g_dbus_client_set_connect_watch(client, connect_handler,
NULL);
> > > > +	g_dbus_client_set_disconnect_watch(client,
disconnect_handler,
> > > > +NULL);
> > >
> > > When exactly would disconnect_handler be called in practice? At
> > > least one case seems to be if bluetoothd exits in which case it
> > > seems quite wasteful to try to make any method calls to a
> > > non-existing service. What other scenarios would disconnect_handler
> > > be called in?
> >
> > On running the gatt-service, it registers the service and updates gatt
> > db with the service path.
> > On exiting the gatt-service, it should call "UnregisterService" and
> > clear the gatt service db.
> 
> How exactly does gatt-service exit? If it stops iterating the main loop
your D-
> Bus reply callback would never get called and most of the new code you're
> adding would never get run.

The gatt-service exits on IO interrupts, I have checked gatt-dbus.c, it
cleans up the client db on client disconnect.
So there should not be any disconnect handler for the same in
gatt-service.c. 

> 
> > Otherwise, on next run of gatt-service, when it registers the same
> > service path, it gets an error, service already exists.
> 
> bluetoothd should monitor all of its clients and clean up after them if
they
> exit without unregistering. If that's not happening then it's a bug that's
much
> more important to fix than what you're trying to do here. A quick look at
> src/gatt-dbus.c shows that it's at least trying to clean up, but maybe
there's a
> bug somewhere there.
To register and unregister multiple services, if gatt-service can be
modified so that it provides choice to user for registering/unregistering
multiple time.
Please suggest, if we can add switch case options to iterate through
resgister/unregister service as per the data input to gatt-service.

#gatt-service
Usage: gatt-service [Register/Unregister] [options]

Options:
	Service_UUID
	Included_serv_UUID
	Char_UUID
	Char value
	Desc UUID
	Desc value
Unregister:
	Service_UUID
 
Best Regards,
Bharat