Hi Fred,
> Add bluetooth_ref()/bluetooth_unref() to support reference count in
> bluetooth utils.
didn't you had this as separate patch already? I think we should keep
that as a separate patch.
> ---
> Makefile.am | 2 +-
> plugins/bluetooth.c | 396 ++++++++++++++++++++++++++++++++++++++++++++++++--
> plugins/bluetooth.h | 11 ++
> 3 files changed, 392 insertions(+), 17 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index da59be7..fb3f750 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -312,7 +312,7 @@ builtin_sources += plugins/nokiacdma.c
>
> if BLUETOOTH
> builtin_modules += bluetooth
> -builtin_sources += plugins/bluetooth.c plugins/bluetooth.h
> +builtin_sources += $(btio_sources) plugins/bluetooth.c plugins/bluetooth.h
>
> builtin_modules += hfp
> builtin_sources += plugins/hfp.c plugins/bluetooth.h
Please do it like this:
builtin_sources += $(btio_sources)
builtin_cflags += @BLUEZ_CFLAGS@
builtin_libadd += @BLUEZ_LIBS@
> +struct client {
> + GIOChannel *io;
> + guint watch;
> +};
Why do you need both. Normally only the watch is enough?
> +struct server {
> + gchar *name;
Don't bother with gchar. Just use char.
> + guint8 channel;
> + gchar *sdp_record;
> + GIOChannel *io;
> + gchar *adapter;
> + guint handle;
> + ConnectFunc connect_cb;
> + gpointer user_data;
> +
> + struct client client;
Is it not simpler to just fold client_watch directly into it?
> +};
>
> void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,
> char *buf, int size)
> @@ -370,6 +393,246 @@ static gboolean property_changed(DBusConnection
> *connection, DBusMessage *msg,
> return TRUE;
> }
>
> +static void disconnect(struct server *server)
> +{
> + struct client *client = &server->client;
> +
> + if (!client->io)
> + return;
> +
> + g_io_channel_unref(client->io);
> + client->io = NULL;
> +
> + if (client->watch > 0) {
> + g_source_remove(client->watch);
> + client->watch = 0;
> + }
> +
> + return;
Don't bother with return for functions returning void.
> +}
> +
> +static void server_stop(gpointer data, gpointer user_data)
> +{
> + struct server *server = data;
> +
> + disconnect(server);
> +
> + if (server->handle) {
We do prefer service->handle > 0 for these cases.
> + DBusMessage *msg;
> +
> + msg = dbus_message_new_method_call(BLUEZ_SERVICE,
> + server->adapter,
> + BLUEZ_SERVICE_INTERFACE,
> + "RemoveRecord");
> + dbus_message_append_args(msg, DBUS_TYPE_UINT32, &server->handle,
> + DBUS_TYPE_INVALID);
> + g_dbus_send_message(connection, msg);
> +
> + server->handle = 0;
> + }
> +
> + if (server->io) {
And here please do server->io != NULL.
> + g_io_channel_shutdown(server->io, TRUE, NULL);
> + g_io_channel_unref(server->io);
> + server->io = NULL;
> + }
> +
> + g_free(server->adapter);
> + server->adapter = NULL;
> +}
> +
> +static gboolean client_event(GIOChannel *chan, GIOCondition cond, gpointer
> data)
> +{
> + struct server *server = data;
> +
> + disconnect(server);
> +
> + return FALSE;
> +}
> +
> +static void cancel_authorization(struct server *server)
> +{
> + DBusMessage *msg;
> +
> + if (!server->adapter)
> + return;
> +
> + msg = dbus_message_new_method_call(BLUEZ_SERVICE, server->adapter,
> + BLUEZ_SERVICE_INTERFACE,
> + "CancelAuthorization");
Check that msg allocation succeeded before calling send.
> +
> + g_dbus_send_message(connection, msg);
> +}
> +
> +static void auth_cb(DBusPendingCall *call, gpointer user_data)
> +{
> + struct server *server = user_data;
> + struct client *client = &server->client;
> + GIOChannel *io = client->io;
> + DBusMessage *reply = dbus_pending_call_steal_reply(call);
> + DBusError derr;
> + GError *err = NULL;
> +
> + dbus_error_init(&derr);
> +
> + if (dbus_set_error_from_message(&derr, reply)) {
> + ofono_error("RequestAuthorization error: %s, %s",
> + derr.name, derr.message);
> +
> + if (dbus_error_has_name(&derr, DBUS_ERROR_NO_REPLY))
> + cancel_authorization(server);
> +
> + dbus_error_free(&derr);
> + goto failed;
> + }
> +
> + ofono_info("RequestAuthorization succeeded");
> +
> + if (!bt_io_accept(io, server->connect_cb, server->user_data,
> + NULL, &err)) {
> + ofono_error("%s", err->message);
> + g_error_free(err);
> + goto failed;
> + }
> +
> + g_source_remove(client->watch);
> +
> + client->watch = g_io_add_watch(client->io,
> + G_IO_HUP | G_IO_ERR | G_IO_NVAL,
> + client_event, server);
> +
> + dbus_message_unref(reply);
You could move dbus_message_unref higher up here since we are not
referencing anything in that message anymore.
Only if you access parameters or error values, you need to keep it
around.
> +
> + return;
> +
> +failed:
> + dbus_message_unref(reply);
> + disconnect(server);
> +}
> +
> +static gboolean auth_watch(GIOChannel *io, GIOCondition cond,
> + gpointer user_data)
> +{
> + struct server *server = user_data;
> +
> + cancel_authorization(server);
> + disconnect(server);
> +
> + return FALSE;
> +}
> +
> +static void confirm_event(GIOChannel *io, gpointer user_data)
> +{
> + struct server *server = user_data;
> + struct client *client = &server->client;
> + GError *err = NULL;
> + char address[18];
> + const char *addr;
> + guint8 channel;
> + int ret;
> +
> + if (client->io) {
Check with io != NULL.
> + ofono_error("Rejecting connection since one client already \
> + connected");
> + return;
> + }
> +
> + bt_io_get(io, BT_IO_RFCOMM, &err, BT_IO_OPT_DEST, address,
> + BT_IO_OPT_CHANNEL, &channel,
> + BT_IO_OPT_INVALID);
> + if (err) {
> + ofono_error("%s", err->message);
> + g_error_free(err);
> + return;
> + }
> +
> + ofono_info("New connection from: %s, channel %u", address, channel);
> +
> + addr = address;
> + ret = bluetooth_send_with_reply(server->adapter,
> + BLUEZ_SERVICE_INTERFACE,
> + "RequestAuthorization",
> + auth_cb, server, NULL, TIMEOUT,
> + DBUS_TYPE_STRING, &addr,
> + DBUS_TYPE_UINT32, &server->handle,
> + DBUS_TYPE_INVALID);
> + if (ret < 0) {
> + ofono_error("Request Bluetooth authorization failed");
> + return;
> + }
> +
> + ofono_info("RequestAuthorization(%s, 0x%x)", address, server->handle);
> +
> + client->io = g_io_channel_ref(io);
> + client->watch = g_io_add_watch(io, G_IO_HUP | G_IO_ERR | G_IO_NVAL,
> + auth_watch, server);
You know that add_watch takes a GIOChannel reference count already. Why
take another one?
> +
> + return;
No return for void functions.
> +}
> +
> +static void add_record_cb(DBusPendingCall *call, gpointer user_data)
> +{
> + struct server *server = user_data;
> + DBusMessage *reply = dbus_pending_call_steal_reply(call);
> + DBusError derr;
> + guint32 handle;
> +
> + dbus_error_init(&derr);
> +
> + if (dbus_set_error_from_message(&derr, reply)) {
> + ofono_error("Replied with an error: %s, %s",
> + derr.name, derr.message);
> + dbus_error_free(&derr);
> + server_stop(server, NULL);
> + goto done;
> + }
> +
> + dbus_message_get_args(reply, NULL, DBUS_TYPE_UINT32, &handle,
> + DBUS_TYPE_INVALID);
> + server->handle = handle;
> +
> + ofono_info("Registered: %s, handle: 0x%x", server->name, handle);
> +
> +done:
> + dbus_message_unref(reply);
> +}
> +
> +static void server_start(gpointer data, gpointer user_data)
> +{
> + struct server *server = data;
> + gchar *path = user_data;
> + GError *err = NULL;
> +
> + if (server->io != NULL)
> + return;
> +
> + server->io = bt_io_listen(BT_IO_RFCOMM, NULL, confirm_event,
> + server, NULL, &err,
> + BT_IO_OPT_CHANNEL, server->channel,
> + BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
> + BT_IO_OPT_INVALID);
> + if (server->io == NULL) {
> + ofono_error("Bluetooth %s register failed: %s",
> + server->name, err->message);
> + g_error_free(err);
> + goto failed;
> + }
> +
> + server->adapter = g_strdup(path);
> +
> + if (server->sdp_record != NULL)
> + bluetooth_send_with_reply(path, BLUEZ_SERVICE_INTERFACE,
> + "AddRecord", add_record_cb,
> + server, NULL, -1,
> + DBUS_TYPE_STRING, &server->sdp_record,
> + DBUS_TYPE_INVALID);
> +
> + return;
> +
> +failed:
> + server_stop(server, NULL);
Since this is only used in one error branch. You could call it directly
and avoid this label.
> +}
> +
> static void adapter_properties_cb(DBusPendingCall *call, gpointer user_data)
> {
> const char *path = user_data;
> @@ -394,6 +657,9 @@ static void adapter_properties_cb(DBusPendingCall *call,
> gpointer user_data)
> g_hash_table_insert(adapter_address_hash,
> g_strdup(path), g_strdup(addr));
>
> + if (server_list)
> + g_slist_foreach(server_list, server_start, (gpointer)path);
> +
> for (l = device_list; l; l = l->next) {
> const char *device = l->data;
>
> @@ -428,11 +694,26 @@ static gboolean adapter_removed(DBusConnection
> *connection,
> DBusMessage *message, void *user_data)
> {
> const char *path;
> + GSList *l;
>
> if (dbus_message_get_args(message, NULL, DBUS_TYPE_OBJECT_PATH, &path,
> DBUS_TYPE_INVALID) == TRUE)
> g_hash_table_remove(adapter_address_hash, path);
>
> + for (l = server_list; l; l = l->next) {
> + struct server *server = l->data;
> +
> + if (!server->adapter)
> + continue;
> +
> + if (!g_str_equal(path, server->adapter))
> + continue;
> +
> + /* Don't remove handle if the adapter has been removed */
> + server->handle = 0;
> + server_stop(server, NULL);
> + }
> +
> return TRUE;
> }
>
> @@ -504,12 +785,14 @@ static guint adapter_added_watch;
> static guint adapter_removed_watch;
> static guint property_watch;
>
> -int bluetooth_register_uuid(const char *uuid, struct bluetooth_profile
> *profile)
> +static int bluetooth_ref()
Empty parameter list need to be declared as (void).
> {
> int err;
>
> - if (uuid_hash)
> - goto done;
> + g_atomic_int_inc(&ref_count);
> +
> + if (ref_count > 1)
> + return 0;
>
> connection = ofono_dbus_get_connection();
>
> @@ -543,13 +826,6 @@ int bluetooth_register_uuid(const char *uuid, struct
> bluetooth_profile *profile)
> adapter_address_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
> g_free, g_free);
>
> -done:
> - g_hash_table_insert(uuid_hash, g_strdup(uuid), profile);
> -
> - bluetooth_send_with_reply("/", BLUEZ_MANAGER_INTERFACE, "GetProperties",
> - manager_properties_cb, NULL, NULL, -1,
> - DBUS_TYPE_INVALID);
> -
> return 0;
>
> remove:
> @@ -557,14 +833,18 @@ remove:
> g_dbus_remove_watch(connection, adapter_added_watch);
> g_dbus_remove_watch(connection, adapter_removed_watch);
> g_dbus_remove_watch(connection, property_watch);
> +
> return err;
> }
>
> -void bluetooth_unregister_uuid(const char *uuid)
> +static void bluetooth_unref()
Use (void) here as well.
> {
> - g_hash_table_remove(uuid_hash, uuid);
> + gboolean is_zero;
> + GSList *l;
> +
> + is_zero = g_atomic_int_dec_and_test(&ref_count);
>
> - if (g_hash_table_size(uuid_hash))
> + if (is_zero == FALSE)
> return;
>
> g_dbus_remove_watch(connection, bluetooth_watch);
> @@ -572,9 +852,93 @@ void bluetooth_unregister_uuid(const char *uuid)
> g_dbus_remove_watch(connection, adapter_removed_watch);
> g_dbus_remove_watch(connection, property_watch);
>
> - g_hash_table_destroy(uuid_hash);
> - g_hash_table_destroy(adapter_address_hash);
> - uuid_hash = NULL;
> + if (uuid_hash) {
> + g_hash_table_destroy(uuid_hash);
> + uuid_hash = NULL;
> + }
> +
> + if (adapter_address_hash) {
> + g_hash_table_destroy(adapter_address_hash);
> + adapter_address_hash = NULL;
> + }
> +
> + if (server_list == NULL)
> + return;
> +
> + for (l = server_list; l; l = l->next) {
> + struct server *server = l->data;
> +
> + server_stop(server, NULL);
> + g_free(server->name);
> + g_free(server);
> + }
> +
> + g_slist_free(server_list);
> + server_list = NULL;
> +}
> +
> +int bluetooth_register_uuid(const char *uuid, struct bluetooth_profile
> *profile)
> +{
> + int err = bluetooth_ref();;
Double semicolon?
And err with ref. This doesn't sound right. Please send the ref/unref
patch first for initial review.
> +
> + if (err != 0)
> + return err;
> +
> + g_hash_table_insert(uuid_hash, g_strdup(uuid), profile);
> +
> + bluetooth_send_with_reply("/", BLUEZ_MANAGER_INTERFACE, "GetProperties",
> + manager_properties_cb, NULL, NULL, -1,
> + DBUS_TYPE_INVALID);
> +
> + return 0;
> +}
> +
> +void bluetooth_unregister_uuid(const char *uuid)
> +{
> + g_hash_table_remove(uuid_hash, uuid);
> +
> + bluetooth_unref();
> +}
> +
> +struct server *bluetooth_register_rfcomm_server(char *name, guint8 channel,
> + const char *sdp_record, ConnectFunc cb,
> + gpointer user_data)
> +{
> + struct server *server;
> + int err = bluetooth_ref();
> +
> + if (err != 0)
> + return NULL;
> +
> + server = g_try_new0(struct server, 1);
> + if (!server)
> + return NULL;
> +
> + server->name = g_strdup(name);
> + server->channel = channel;
> + if (sdp_record != NULL)
> + server->sdp_record = g_strdup(sdp_record);
> + server->connect_cb = cb;
> + server->user_data = user_data;
> +
> + server_list = g_slist_prepend(server_list, server);
> +
> + bluetooth_send_with_reply("/", BLUEZ_MANAGER_INTERFACE, "GetProperties",
> + manager_properties_cb, NULL, NULL, -1,
> + DBUS_TYPE_INVALID);
> +
> + return server;
> +}
> +
> +void bluetooth_unregister_rfcomm_server(struct server *server)
> +{
> + server_list = g_slist_remove(server_list, server);
> + server_stop(server, NULL);
> + g_free(server->name);
> + g_free(server->sdp_record);
> + g_free(server);
> +
> + bluetooth_unref();
> }
>
> OFONO_PLUGIN_DEFINE(bluetooth, "Bluetooth Utils Plugins", VERSION,
> diff --git a/plugins/bluetooth.h b/plugins/bluetooth.h
> index 42b0d13..c0df13f 100644
> --- a/plugins/bluetooth.h
> +++ b/plugins/bluetooth.h
> @@ -3,6 +3,7 @@
> * oFono - Open Source Telephony
> *
> * Copyright (C) 2010 Gustavo F. Padovan <[email protected]>
> + * Copyright (C) 2010 Intel Corporation. All rights reserved.
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -23,6 +24,7 @@
> #define BLUEZ_MANAGER_INTERFACE BLUEZ_SERVICE ".Manager"
> #define BLUEZ_ADAPTER_INTERFACE BLUEZ_SERVICE ".Adapter"
> #define BLUEZ_DEVICE_INTERFACE BLUEZ_SERVICE ".Device"
> +#define BLUEZ_SERVICE_INTERFACE BLUEZ_SERVICE ".Service"
>
> #define DBUS_TIMEOUT 15
>
> @@ -39,10 +41,19 @@ struct bluetooth_profile {
> void (*set_alias)(const char *device, const char *);
> };
>
> +struct server;
> +
> +typedef void (*ConnectFunc)(GIOChannel *io, GError *err, gpointer user_data);
> +
> int bluetooth_register_uuid(const char *uuid,
> struct bluetooth_profile *profile);
> void bluetooth_unregister_uuid(const char *uuid);
>
> +struct server *bluetooth_register_rfcomm_server(char *name, guint8 channel,
> + const char *sdp_record, ConnectFunc cb,
> + gpointer user_data);
> +void bluetooth_unregister_rfcomm_server(struct server *server);
> +
> void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,
> char *buf, int size);
>
This was just an initial review, we really need to split this one in
more digestible pieces.
Regards
Marcel
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
