2011-06-27 21:39:35

by Dmitriy Paliy

[permalink] [raw]
Subject: [RFC] Add adapter session request/release to obex-client

Hi,

This set of patches adds system bus connection to obex-client and
handling of adapter's method calls, among others, as
RequestSession and ReleaseSession.

Such ensures OBEX transfers done within a single adapter session,
which in turn significantly simplifies file transfer API for clients
of obex-client.

Any comments would be appreciated.

BR,
Dmitriy



2011-06-28 07:59:47

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 6/6] Code clean-up: remove unnecessary brackets

Hi,

On Tue, Jun 28, 2011 at 12:39 AM, Dmitriy Paliy <[email protected]> wrote:
> ---
> ?client/session.c | ? ?3 +--
> ?1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/client/session.c b/client/session.c
> index d8e3bf2..ae7e213 100644
> --- a/client/session.c
> +++ b/client/session.c
> @@ -241,9 +241,8 @@ static void session_free(struct session_data *session)
> ? ? ? ?if (session->path)
> ? ? ? ? ? ? ? ?session_unregistered(session);
>
> - ? ? ? if (session->conn) {
> + ? ? ? if (session->conn)
> ? ? ? ? ? ? ? ?dbus_connection_unref(session->conn);
> - ? ? ? }
>
> ? ? ? ?if (session->conn_system)
> ? ? ? ? ? ? ? ?dbus_connection_unref(session->conn_system);
> --
> 1.7.4.1
>
>

This patch is fine, but I would like to suggest that we only include
in the series patches that depend on each other, this one we could
quickly apply without looking into the others. Btw, we started using
project names on the subject prefix to help identifying what project
the patch is for, you can do that by editing the .git/config where you
cloned the repository adding the following:

[format]
subjectprefix = PATCH obexd


--
Luiz Augusto von Dentz

2011-06-28 07:48:11

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 5/6] Add FindAdapter method call to obex-client

Hi Dmitriy,

On Tue, Jun 28, 2011 at 12:39 AM, Dmitriy Paliy <[email protected]> wrote:
> ---
> ?client/session.c | ? 12 +++++++++++-
> ?1 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/client/session.c b/client/session.c
> index 423eb8c..d8e3bf2 100644
> --- a/client/session.c
> +++ b/client/session.c
> @@ -848,11 +848,21 @@ proceed:
> ? ? ? ?callback->func = function;
> ? ? ? ?callback->data = user_data;
>
> - ? ? ? req = send_method_call(session->conn_system,
> + ? ? ? if (source) {
> + ? ? ? ? ? ? ? req = send_method_call(session->conn_system,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? BT_BUS_NAME, BT_PATH,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? BT_MANAGER_IFACE, "FindAdapter",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? manager_reply, callback,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DBUS_TYPE_STRING, &source,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DBUS_TYPE_INVALID);
> + ? ? ? } else {
> + ? ? ? ? ? ? ? req = send_method_call(session->conn_system,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?BT_BUS_NAME, BT_PATH,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?BT_MANAGER_IFACE, "DefaultAdapter",
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?manager_reply, callback,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?DBUS_TYPE_INVALID);
> + ? ? ? }
> +
> ? ? ? ?if (!req) {
> ? ? ? ? ? ? ? ?session_unref(session);
> ? ? ? ? ? ? ? ?g_free(callback);
> --
> 1.7.4.1

I guess this one belong to the same patch where DefaultAdapter call
was introduced since it actually fix requesting the session to the
wrong adapter in case it the address is not from the default.


--
Luiz Augusto von Dentz

2011-06-28 07:42:36

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 4/6] Add adapter ReleaseSession to obex-client

Hi Dmitriy,

On Tue, Jun 28, 2011 at 12:39 AM, Dmitriy Paliy <[email protected]> wrote:
> Release of adapter session is added to obex-client upon closing OBEX
> transfer.
> ---
> ?client/session.c | ? 38 +++++++++++++++++++++++++++++++++++++-
> ?1 files changed, 37 insertions(+), 1 deletions(-)
>
> diff --git a/client/session.c b/client/session.c
> index 75cd32e..423eb8c 100644
> --- a/client/session.c
> +++ b/client/session.c
> @@ -302,9 +302,34 @@ static struct pending_req *send_method_call(DBusConnection *connection,
> ? ? ? ?return req;
> ?}
>
> +static void adapter_release_reply(DBusPendingCall *call, void *user_data)
> +{
> + ? ? ? DBusError err;
> + ? ? ? DBusMessage *reply;
> + ? ? ? struct session_data *session = user_data;
> + ? ? ? struct pending_req *req = find_session_request(session, call);
> +
> + ? ? ? reply = dbus_pending_call_steal_reply(call);
> +
> + ? ? ? dbus_error_init(&err);
> + ? ? ? if (dbus_set_error_from_message(&err, reply)) {
> + ? ? ? ? ? ? ? error("manager replied with an error: %s, %s",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? err.name, err.message);
> + ? ? ? ? ? ? ? dbus_error_free(&err);
> + ? ? ? }
> +
> + ? ? ? dbus_message_unref(reply);
> +
> + ? ? ? session->pending_calls = g_slist_remove(session->pending_calls, req);
> + ? ? ? pending_req_finalize(req);
> +
> + ? ? ? session_free(session);
> +}
> +
> ?void session_unref(struct session_data *session)
> ?{
> ? ? ? ?gboolean ret;
> + ? ? ? struct pending_req *req;
>
> ? ? ? ?ret = g_atomic_int_dec_and_test(&session->refcount);
>
> @@ -313,7 +338,18 @@ void session_unref(struct session_data *session)
> ? ? ? ?if (ret == FALSE)
> ? ? ? ? ? ? ? ?return;
>
> - ? ? ? session_free(session);
> + ? ? ? req = send_method_call(session->conn_system,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? BT_BUS_NAME, session->adapter,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? BT_ADAPTER_IFACE, "ReleaseSession",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? adapter_release_reply, session,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DBUS_TYPE_INVALID);
> + ? ? ? if (!req) {
> + ? ? ? ? ? ? ? g_free(session->adapter);
> + ? ? ? ? ? ? ? session_free(session);
> + ? ? ? ? ? ? ? return;
> + ? ? ? }
> +
> + ? ? ? session->pending_calls = g_slist_prepend(session->pending_calls, req);
> ?}
>
> ?static void rfcomm_callback(GIOChannel *io, GError *err, gpointer user_data)
> --
> 1.7.4.1
>
>

I don't think this is really necessary since you basically gonna
disconnect from system bus on session_free, so calling ReleaseSession
and wait for the reply will only add complexity and it also leaves the
session with 0 refcount which I don't think is a good idea. In the
other hand we may want to stay connected to the system bus so that we
don't have to reconnect every time when creating a new session, in
that case you would need to call ReleaseSession but I wouldn't bother
waiting for the reply since ReleaseSession only fails in case the
process doesn't own any session which is still fine since it doesn't
leave anything behind.

--
Luiz Augusto von Dentz

2011-06-27 21:39:40

by Dmitriy Paliy

[permalink] [raw]
Subject: [PATCH 5/6] Add FindAdapter method call to obex-client

---
client/session.c | 12 +++++++++++-
1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/client/session.c b/client/session.c
index 423eb8c..d8e3bf2 100644
--- a/client/session.c
+++ b/client/session.c
@@ -848,11 +848,21 @@ proceed:
callback->func = function;
callback->data = user_data;

- req = send_method_call(session->conn_system,
+ if (source) {
+ req = send_method_call(session->conn_system,
+ BT_BUS_NAME, BT_PATH,
+ BT_MANAGER_IFACE, "FindAdapter",
+ manager_reply, callback,
+ DBUS_TYPE_STRING, &source,
+ DBUS_TYPE_INVALID);
+ } else {
+ req = send_method_call(session->conn_system,
BT_BUS_NAME, BT_PATH,
BT_MANAGER_IFACE, "DefaultAdapter",
manager_reply, callback,
DBUS_TYPE_INVALID);
+ }
+
if (!req) {
session_unref(session);
g_free(callback);
--
1.7.4.1


2011-06-27 21:39:39

by Dmitriy Paliy

[permalink] [raw]
Subject: [PATCH 4/6] Add adapter ReleaseSession to obex-client

Release of adapter session is added to obex-client upon closing OBEX
transfer.
---
client/session.c | 38 +++++++++++++++++++++++++++++++++++++-
1 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/client/session.c b/client/session.c
index 75cd32e..423eb8c 100644
--- a/client/session.c
+++ b/client/session.c
@@ -302,9 +302,34 @@ static struct pending_req *send_method_call(DBusConnection *connection,
return req;
}

+static void adapter_release_reply(DBusPendingCall *call, void *user_data)
+{
+ DBusError err;
+ DBusMessage *reply;
+ struct session_data *session = user_data;
+ struct pending_req *req = find_session_request(session, call);
+
+ reply = dbus_pending_call_steal_reply(call);
+
+ dbus_error_init(&err);
+ if (dbus_set_error_from_message(&err, reply)) {
+ error("manager replied with an error: %s, %s",
+ err.name, err.message);
+ dbus_error_free(&err);
+ }
+
+ dbus_message_unref(reply);
+
+ session->pending_calls = g_slist_remove(session->pending_calls, req);
+ pending_req_finalize(req);
+
+ session_free(session);
+}
+
void session_unref(struct session_data *session)
{
gboolean ret;
+ struct pending_req *req;

ret = g_atomic_int_dec_and_test(&session->refcount);

@@ -313,7 +338,18 @@ void session_unref(struct session_data *session)
if (ret == FALSE)
return;

- session_free(session);
+ req = send_method_call(session->conn_system,
+ BT_BUS_NAME, session->adapter,
+ BT_ADAPTER_IFACE, "ReleaseSession",
+ adapter_release_reply, session,
+ DBUS_TYPE_INVALID);
+ if (!req) {
+ g_free(session->adapter);
+ session_free(session);
+ return;
+ }
+
+ session->pending_calls = g_slist_prepend(session->pending_calls, req);
}

static void rfcomm_callback(GIOChannel *io, GError *err, gpointer user_data)
--
1.7.4.1


2011-06-27 21:39:41

by Dmitriy Paliy

[permalink] [raw]
Subject: [PATCH 6/6] Code clean-up: remove unnecessary brackets

---
client/session.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/client/session.c b/client/session.c
index d8e3bf2..ae7e213 100644
--- a/client/session.c
+++ b/client/session.c
@@ -241,9 +241,8 @@ static void session_free(struct session_data *session)
if (session->path)
session_unregistered(session);

- if (session->conn) {
+ if (session->conn)
dbus_connection_unref(session->conn);
- }

if (session->conn_system)
dbus_connection_unref(session->conn_system);
--
1.7.4.1


2011-06-27 21:39:38

by Dmitriy Paliy

[permalink] [raw]
Subject: [PATCH 3/6] Add handling of system D-Bus method calls

Sending system D-Bus method calls (DefaultAdapter and RequestSession)
and handling of pending D-Bus calls is added to obex-client.
---
client/session.c | 186 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
client/session.h | 2 +
2 files changed, 187 insertions(+), 1 deletions(-)

diff --git a/client/session.c b/client/session.c
index 4016e35..75cd32e 100644
--- a/client/session.c
+++ b/client/session.c
@@ -56,6 +56,11 @@

#define OBEX_IO_ERROR obex_io_error_quark()

+#define BT_BUS_NAME "org.bluez"
+#define BT_PATH "/"
+#define BT_ADAPTER_IFACE "org.bluez.Adapter"
+#define BT_MANAGER_IFACE "org.bluez.Manager"
+
static guint64 counter = 0;

static unsigned char pcsuite_uuid[] = { 0x00, 0x00, 0x50, 0x05, 0x00, 0x00,
@@ -87,6 +92,11 @@ struct agent_data {
struct pending_data *pending;
};

+struct pending_req {
+ DBusPendingCall *call;
+ void *user_data;
+};
+
static GSList *sessions = NULL;

static void session_prepare_put(struct session_data *session, GError *err,
@@ -176,10 +186,44 @@ static void session_unregistered(struct session_data *session)
session->path = NULL;
}

+struct pending_req *find_session_request(const struct session_data *session,
+ const DBusPendingCall *call)
+{
+ GSList *l;
+
+ for (l = session->pending_calls; l; l = l->next) {
+ struct pending_req *req = l->data;
+
+ if (req->call == call)
+ return req;
+ }
+
+ return NULL;
+}
+
+static void pending_req_finalize(struct pending_req *req)
+{
+ if (!dbus_pending_call_get_completed(req->call))
+ dbus_pending_call_cancel(req->call);
+
+ dbus_pending_call_unref(req->call);
+ g_free(req);
+}
+
static void session_free(struct session_data *session)
{
+ GSList *l = session->pending_calls;
+
DBG("%p", session);

+ while (l) {
+ struct pending_req *req = l->data;
+ l = l->next;
+
+ session->pending_calls = g_slist_remove(session->pending_calls, req);
+ pending_req_finalize(req);
+ }
+
if (session->agent)
agent_release(session);

@@ -206,6 +250,7 @@ static void session_free(struct session_data *session)

sessions = g_slist_remove(sessions, session);

+ g_free(session->adapter);
g_free(session->callback);
g_free(session->path);
g_free(session->service);
@@ -213,6 +258,50 @@ static void session_free(struct session_data *session)
g_free(session);
}

+static struct pending_req *send_method_call(DBusConnection *connection,
+ const char *dest, const char *path,
+ const char *interface, const char *method,
+ DBusPendingCallNotifyFunction cb,
+ void *user_data, int type, ...)
+{
+ DBusMessage *msg;
+ DBusPendingCall *call;
+ va_list args;
+ struct pending_req *req;
+
+ msg = dbus_message_new_method_call(dest, path, interface, method);
+ if (!msg) {
+ error("Unable to allocate new D-Bus %s message", method);
+ return NULL;
+ }
+
+ va_start(args, type);
+
+ if (!dbus_message_append_args_valist(msg, type, args)) {
+ dbus_message_unref(msg);
+ va_end(args);
+ return NULL;
+ }
+
+ va_end(args);
+
+ if (!dbus_connection_send_with_reply(connection, msg, &call, -1)) {
+ error("Sending %s failed", method);
+ dbus_message_unref(msg);
+ return NULL;
+ }
+
+ dbus_pending_call_set_notify(call, cb, user_data, NULL);
+
+ req = g_new0(struct pending_req, 1);
+ req->call = call;
+ req->user_data = user_data;
+
+ dbus_message_unref(msg);
+
+ return req;
+}
+
void session_unref(struct session_data *session)
{
gboolean ret;
@@ -557,6 +646,93 @@ static int session_connect(struct session_data *session,
return err;
}

+static void adapter_request_reply(DBusPendingCall *call, void *user_data)
+{
+ DBusError err;
+ DBusMessage *reply;
+ struct callback_data *callback = user_data;
+ struct session_data *session = callback->session;
+ struct pending_req *req = find_session_request(session, call);
+
+ reply = dbus_pending_call_steal_reply(call);
+
+ session->pending_calls = g_slist_remove(session->pending_calls, req);
+ pending_req_finalize(req);
+
+ dbus_error_init(&err);
+ if (dbus_set_error_from_message(&err, reply)) {
+ error("manager replied with an error: %s, %s",
+ err.name, err.message);
+ dbus_error_free(&err);
+
+ goto failed;
+ }
+
+ if (session_connect(session, callback) < 0)
+ goto failed;
+
+ goto proceed;
+
+failed:
+ session_unref(session);
+ g_free(callback);
+
+proceed:
+ dbus_message_unref(reply);
+}
+
+static void manager_reply(DBusPendingCall *call, void *user_data)
+{
+ DBusError err;
+ DBusMessage *reply;
+ char *adapter;
+ struct callback_data *callback = user_data;
+ struct session_data *session = callback->session;
+ struct pending_req *req = find_session_request(session, call);
+
+ reply = dbus_pending_call_steal_reply(call);
+
+ session->pending_calls = g_slist_remove(session->pending_calls, req);
+ pending_req_finalize(req);
+
+ dbus_error_init(&err);
+ if (dbus_set_error_from_message(&err, reply)) {
+ error("manager replied with an error: %s, %s",
+ err.name, err.message);
+ dbus_error_free(&err);
+
+ goto failed;
+ }
+
+ if (dbus_message_get_args(reply, NULL,
+ DBUS_TYPE_OBJECT_PATH, &adapter,
+ DBUS_TYPE_INVALID)) {
+ DBG("adapter path %s", adapter);
+
+ session->adapter = g_strdup(adapter);
+ req = send_method_call(session->conn_system,
+ BT_BUS_NAME, adapter,
+ BT_ADAPTER_IFACE, "RequestSession",
+ adapter_request_reply, callback,
+ DBUS_TYPE_INVALID);
+ if (!req)
+ goto failed;
+
+ session->pending_calls = g_slist_prepend(session->pending_calls,
+ req);
+ } else
+ goto failed;
+
+ goto proceed;
+
+failed:
+ session_unref(session);
+ g_free(callback);
+
+proceed:
+ dbus_message_unref(reply);
+}
+
struct session_data *session_create(const char *source,
const char *destination,
const char *service,
@@ -567,6 +743,7 @@ struct session_data *session_create(const char *source,
{
struct session_data *session;
struct callback_data *callback;
+ struct pending_req *req;

if (destination == NULL)
return NULL;
@@ -635,12 +812,19 @@ proceed:
callback->func = function;
callback->data = user_data;

- if (session_connect(session, callback) < 0) {
+ req = send_method_call(session->conn_system,
+ BT_BUS_NAME, BT_PATH,
+ BT_MANAGER_IFACE, "DefaultAdapter",
+ manager_reply, callback,
+ DBUS_TYPE_INVALID);
+ if (!req) {
session_unref(session);
g_free(callback);
return NULL;
}

+ session->pending_calls = g_slist_prepend(session->pending_calls, req);
+
if (owner)
session_set_owner(session, owner, owner_disconnected);

diff --git a/client/session.h b/client/session.h
index 554b494..39f5742 100644
--- a/client/session.h
+++ b/client/session.h
@@ -51,7 +51,9 @@ struct session_data {
gchar *owner; /* Session owner */
guint watch;
GSList *pending;
+ GSList *pending_calls;
void *priv;
+ char *adapter;
};

typedef void (*session_callback_t) (struct session_data *session,
--
1.7.4.1


2011-06-27 21:39:37

by Dmitriy Paliy

[permalink] [raw]
Subject: [PATCH 2/6] Split up session_create in separate functions

Connection of RFCOMM and SDP are extracted from session_create function
into session_connect. Such allows making asynchronous calls before
creating connections.
---
client/session.c | 41 ++++++++++++++++++++++++-----------------
1 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/client/session.c b/client/session.c
index 4a4922f..4016e35 100644
--- a/client/session.c
+++ b/client/session.c
@@ -534,6 +534,29 @@ static struct session_data *session_find(const char *source,
return NULL;
}

+static int session_connect(struct session_data *session,
+ struct callback_data *callback)
+{
+ int err;
+
+ if (session->obex) {
+ g_idle_add(connection_complete, callback);
+ err = 0;
+ } else if (session->channel > 0) {
+ session->io = rfcomm_connect(&session->src, &session->dst,
+ session->channel,
+ rfcomm_callback,
+ callback);
+ err = (session->io == NULL) ? -EINVAL : 0;
+ } else {
+ callback->sdp = service_connect(&session->src, &session->dst,
+ service_callback, callback);
+ err = (callback->sdp == NULL) ? -ENOMEM : 0;
+ }
+
+ return err;
+}
+
struct session_data *session_create(const char *source,
const char *destination,
const char *service,
@@ -544,7 +567,6 @@ struct session_data *session_create(const char *source,
{
struct session_data *session;
struct callback_data *callback;
- int err;

if (destination == NULL)
return NULL;
@@ -613,22 +635,7 @@ proceed:
callback->func = function;
callback->data = user_data;

- if (session->obex) {
- g_idle_add(connection_complete, callback);
- err = 0;
- } else if (session->channel > 0) {
- session->io = rfcomm_connect(&session->src, &session->dst,
- session->channel,
- rfcomm_callback,
- callback);
- err = (session->io == NULL) ? -EINVAL : 0;
- } else {
- callback->sdp = service_connect(&session->src, &session->dst,
- service_callback, callback);
- err = (callback->sdp == NULL) ? -ENOMEM : 0;
- }
-
- if (err < 0) {
+ if (session_connect(session, callback) < 0) {
session_unref(session);
g_free(callback);
return NULL;
--
1.7.4.1


2011-06-27 21:39:36

by Dmitriy Paliy

[permalink] [raw]
Subject: [PATCH 1/6] Add system bus connection in obex-client

Connection to system bus is added in obex-client. Purpose is to carry
out OBEX transfers within a single adapter's session.
---
client/session.c | 9 +++++++++
client/session.h | 1 +
2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/client/session.c b/client/session.c
index 266d00c..4a4922f 100644
--- a/client/session.c
+++ b/client/session.c
@@ -201,6 +201,9 @@ static void session_free(struct session_data *session)
dbus_connection_unref(session->conn);
}

+ if (session->conn_system)
+ dbus_connection_unref(session->conn_system);
+
sessions = g_slist_remove(sessions, session);

g_free(session->callback);
@@ -565,6 +568,12 @@ struct session_data *session_create(const char *source,
return NULL;
}

+ session->conn_system = g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, NULL);
+ if (session->conn_system == NULL) {
+ session_free(session);
+ return NULL;
+ }
+
if (source == NULL)
bacpy(&session->src, BDADDR_ANY);
else
diff --git a/client/session.h b/client/session.h
index 6f8a434..554b494 100644
--- a/client/session.h
+++ b/client/session.h
@@ -42,6 +42,7 @@ struct session_data {
uuid_t uuid; /* Bluetooth Service Class */
gchar *path; /* Session path */
DBusConnection *conn;
+ DBusConnection *conn_system; /* system bus connection */
DBusMessage *msg;
GwObex *obex;
GIOChannel *io;
--
1.7.4.1