linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH obexd v2 0/8] Transfer cancelation
@ 2012-02-21 13:57 Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 1/8] gobex: fix callback remove when canceling transfer Mikel Astiz
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This patch series proposes several fixes regarding the cancelation of transfers.

The goal is that the Cancel method defined in the transfer's D-Bus API should work for any registered transfer, be it in progress or queued. Currently none of them seem to work properly.

This second version is mostly a refacting of the previous proposal, according to the review from Luiz. The exceptions are patch 4/9 which has now been dropped, and patch 7/9, which now prevents from setting the callback twice.

Mikel Astiz (8):
  gobex: fix callback remove when canceling transfer
  client: fix obc_session_get_buffer
  client: fix cancel when no agent present
  client: process transfer queue only if none active
  client: terminate queued transfers properly
  client: expose obc_transfer_set_callback
  client: fix canceling queued transfers
  client: make sure callback does not match size

 client/session.c  |   45 ++++++++++++++++++++++++++++++++++++++-------
 client/transfer.c |   37 ++++++++++++++++++-------------------
 client/transfer.h |   10 ++++++----
 gobex/gobex.c     |    3 +++
 4 files changed, 65 insertions(+), 30 deletions(-)

-- 
1.7.6.5


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH obexd v2 1/8] gobex: fix callback remove when canceling transfer
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
@ 2012-02-21 13:57 ` Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 2/8] client: fix obc_session_get_buffer Mikel Astiz
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This code path could lead to situations where the callback is later
used, making the daemon crash.
---
 gobex/gobex.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gobex/gobex.c b/gobex/gobex.c
index 0665749..bc76e57 100644
--- a/gobex/gobex.c
+++ b/gobex/gobex.c
@@ -735,6 +735,9 @@ gboolean g_obex_cancel_req(GObex *obex, guint req_id, gboolean remove_callback)
 			goto immediate_completion;
 		}
 
+		if (remove_callback)
+			obex->pending_req->rsp_func = NULL;
+
 		return TRUE;
 	}
 
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH obexd v2 2/8] client: fix obc_session_get_buffer
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 1/8] gobex: fix callback remove when canceling transfer Mikel Astiz
@ 2012-02-21 13:57 ` Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 3/8] client: fix cancel when no agent present Mikel Astiz
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Size 0 should be reported if no transfer exists. Some existing code
relies on this behavior.
---
 client/session.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/client/session.c b/client/session.c
index 85f466a..e113d1f 100644
--- a/client/session.c
+++ b/client/session.c
@@ -1128,8 +1128,12 @@ const char *obc_session_get_buffer(struct obc_session *session, size_t *size)
 	const char *buf;
 
 	transfer = obc_session_get_transfer(session);
-	if (transfer == NULL)
+	if (transfer == NULL) {
+		if (size != NULL)
+			*size = 0;
+
 		return NULL;
+	}
 
 	buf = obc_transfer_get_buffer(transfer, size);
 
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH obexd v2 3/8] client: fix cancel when no agent present
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 1/8] gobex: fix callback remove when canceling transfer Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 2/8] client: fix obc_session_get_buffer Mikel Astiz
@ 2012-02-21 13:57 ` Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 4/8] client: process transfer queue only if none active Mikel Astiz
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The authorization check should consider the scenario of no agent being
assigned to the transfer.
---
 client/transfer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/client/transfer.c b/client/transfer.c
index 157811d..4054799 100644
--- a/client/transfer.c
+++ b/client/transfer.c
@@ -168,7 +168,7 @@ static DBusMessage *obc_transfer_cancel(DBusConnection *connection,
 	DBusMessage *reply;
 
 	sender = dbus_message_get_sender(message);
-	if (g_str_equal(transfer->agent, sender) == FALSE)
+	if (g_strcmp0(transfer->agent, sender) != 0)
 		return g_dbus_create_error(message,
 				"org.openobex.Error.NotAuthorized",
 				"Not Authorized");
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH obexd v2 4/8] client: process transfer queue only if none active
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
                   ` (2 preceding siblings ...)
  2012-02-21 13:57 ` [PATCH obexd v2 3/8] client: fix cancel when no agent present Mikel Astiz
@ 2012-02-21 13:57 ` Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 5/8] client: terminate queued transfers properly Mikel Astiz
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

session_process_queue should make sure there is no active operation, to
avoid starting a second one at the same time.
---
 client/session.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/client/session.c b/client/session.c
index e113d1f..7f387d4 100644
--- a/client/session.c
+++ b/client/session.c
@@ -732,6 +732,9 @@ static void session_process_queue(struct obc_session *session)
 {
 	struct pending_request *p;
 
+	if (session->p != NULL)
+		return;
+
 	if (session->queue == NULL || g_queue_is_empty(session->queue))
 		return;
 
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH obexd v2 5/8] client: terminate queued transfers properly
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
                   ` (3 preceding siblings ...)
  2012-02-21 13:57 ` [PATCH obexd v2 4/8] client: process transfer queue only if none active Mikel Astiz
@ 2012-02-21 13:57 ` Mikel Astiz
  2012-02-26 17:23   ` Johan Hedberg
  2012-02-21 13:57 ` [PATCH obexd v2 6/8] client: expose obc_transfer_set_callback Mikel Astiz
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Previous implementation of session_terminate_transfer assumed that the
transfer being terminated would always be the active one. However, it
should be possible to cancel any queued transfer using the D-Bus api.
---
 client/session.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/client/session.c b/client/session.c
index 7f387d4..3429f0a 100644
--- a/client/session.c
+++ b/client/session.c
@@ -764,14 +764,31 @@ static void session_process_queue(struct obc_session *session)
 	obc_session_unref(session);
 }
 
+static gint pending_transfer_cmptransfer(gconstpointer a, gconstpointer b)
+{
+	const struct pending_request *p = a;
+	const struct obc_transfer *transfer = b;
+
+	return p->transfer < transfer ? -1 : (p->transfer > transfer ? 1 : 0);
+}
+
 static void session_terminate_transfer(struct obc_session *session,
 					struct obc_transfer *transfer,
 					GError *gerr)
 {
 	struct pending_request *p = session->p;
 
-	if (p == NULL || p->transfer != transfer)
-		return;
+	if (p == NULL || p->transfer != transfer) {
+		GList *match;
+
+		match = g_list_find_custom(session->queue->head, transfer,
+						pending_transfer_cmptransfer);
+		if (match == NULL)
+			return;
+
+		p = match->data;
+		g_queue_delete_link(session->queue, match);
+	}
 
 	obc_session_ref(session);
 
@@ -779,9 +796,11 @@ static void session_terminate_transfer(struct obc_session *session,
 		p->func(session, gerr, p->data);
 
 	pending_request_free(p);
-	session->p = NULL;
 
-	session_process_queue(session);
+	if (p == session->p) {
+		session->p = NULL;
+		session_process_queue(session);
+	}
 
 	obc_session_unref(session);
 }
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH obexd v2 6/8] client: expose obc_transfer_set_callback
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
                   ` (4 preceding siblings ...)
  2012-02-21 13:57 ` [PATCH obexd v2 5/8] client: terminate queued transfers properly Mikel Astiz
@ 2012-02-21 13:57 ` Mikel Astiz
  2012-02-21 13:57 ` [PATCH obexd v2 7/8] client: fix canceling queued transfers Mikel Astiz
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This will allow setting the callback before the transfer is started,
particularly to report queued transfer cancellations.
---
 client/session.c  |    9 +++++++--
 client/transfer.c |   19 +++++++------------
 client/transfer.h |   10 ++++++----
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/client/session.c b/client/session.c
index 3429f0a..e3b5a56 100644
--- a/client/session.c
+++ b/client/session.c
@@ -107,6 +107,9 @@ static void session_prepare_put(gpointer data, gpointer user_data);
 static void session_terminate_transfer(struct obc_session *session,
 					struct obc_transfer *transfer,
 					GError *gerr);
+static void transfer_progress(struct obc_transfer *transfer,
+					gint64 transferred, GError *err,
+					void *user_data);
 
 GQuark obex_io_error_quark(void)
 {
@@ -710,6 +713,8 @@ static int session_request(struct obc_session *session,
 	struct pending_request *p;
 	int err;
 
+	obc_transfer_set_callback(transfer, transfer_progress, session);
+
 	p = pending_request_new(session, transfer, auth_complete, func, data);
 
 	if (session->p) {
@@ -888,7 +893,7 @@ static void session_prepare_get(gpointer data, gpointer user_data)
 	struct obc_transfer *transfer = user_data;
 	int ret;
 
-	ret = obc_transfer_get(transfer, transfer_progress, session);
+	ret = obc_transfer_get(transfer);
 	if (ret < 0) {
 		GError *gerr = NULL;
 
@@ -1033,7 +1038,7 @@ static void session_prepare_put(gpointer data, gpointer user_data)
 	struct obc_transfer *transfer = user_data;
 	int ret;
 
-	ret = obc_transfer_put(transfer, transfer_progress, session);
+	ret = obc_transfer_put(transfer);
 	if (ret < 0) {
 		GError *gerr = NULL;
 
diff --git a/client/transfer.c b/client/transfer.c
index 4054799..b470a3a 100644
--- a/client/transfer.c
+++ b/client/transfer.c
@@ -488,23 +488,25 @@ static gssize put_xfer_progress(void *buf, gsize len, gpointer user_data)
 	return size;
 }
 
-static void obc_transfer_set_callback(struct obc_transfer *transfer,
+gboolean obc_transfer_set_callback(struct obc_transfer *transfer,
 					transfer_callback_t func,
 					void *user_data)
 {
 	struct transfer_callback *callback;
 
-	g_free(transfer->callback);
+	if (transfer->callback != NULL)
+		return FALSE;
 
 	callback = g_new0(struct transfer_callback, 1);
 	callback->func = func;
 	callback->data = user_data;
 
 	transfer->callback = callback;
+
+	return TRUE;
 }
 
-int obc_transfer_get(struct obc_transfer *transfer, transfer_callback_t func,
-			void *user_data)
+int obc_transfer_get(struct obc_transfer *transfer)
 {
 	GError *err = NULL;
 	GObexPacket *req;
@@ -558,14 +560,10 @@ int obc_transfer_get(struct obc_transfer *transfer, transfer_callback_t func,
 	if (transfer->xfer == 0)
 		return -ENOTCONN;
 
-	if (func)
-		obc_transfer_set_callback(transfer, func, user_data);
-
 	return 0;
 }
 
-int obc_transfer_put(struct obc_transfer *transfer, transfer_callback_t func,
-			void *user_data)
+int obc_transfer_put(struct obc_transfer *transfer)
 {
 	GError *err = NULL;
 	GObexPacket *req;
@@ -606,9 +604,6 @@ done:
 	if (transfer->xfer == 0)
 		return -ENOTCONN;
 
-	if (func)
-		obc_transfer_set_callback(transfer, func, user_data);
-
 	return 0;
 }
 
diff --git a/client/transfer.h b/client/transfer.h
index e5387fc..e7e1000 100644
--- a/client/transfer.h
+++ b/client/transfer.h
@@ -42,10 +42,12 @@ struct obc_transfer *obc_transfer_register(DBusConnection *conn,
 
 void obc_transfer_unregister(struct obc_transfer *transfer);
 
-int obc_transfer_get(struct obc_transfer *transfer, transfer_callback_t func,
-			void *user_data);
-int obc_transfer_put(struct obc_transfer *transfer, transfer_callback_t func,
-			void *user_data);
+gboolean obc_transfer_set_callback(struct obc_transfer *transfer,
+					transfer_callback_t func,
+					void *user_data);
+
+int obc_transfer_get(struct obc_transfer *transfer);
+int obc_transfer_put(struct obc_transfer *transfer);
 
 int obc_transfer_get_params(struct obc_transfer *transfer,
 					struct obc_transfer_params *params);
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH obexd v2 7/8] client: fix canceling queued transfers
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
                   ` (5 preceding siblings ...)
  2012-02-21 13:57 ` [PATCH obexd v2 6/8] client: expose obc_transfer_set_callback Mikel Astiz
@ 2012-02-21 13:57 ` Mikel Astiz
  2012-02-22 11:41   ` Jaganath
  2012-02-26 17:26   ` Johan Hedberg
  2012-02-21 13:57 ` [PATCH obexd v2 8/8] client: make sure callback does not match size Mikel Astiz
  2012-02-24  9:33 ` [PATCH obexd v2 0/8] Transfer cancelation Luiz Augusto von Dentz
  8 siblings, 2 replies; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The Cancel() method in the D-Bus api should also abort queued transfers,
which should just be removed from the queue.
---
 client/transfer.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/client/transfer.c b/client/transfer.c
index b470a3a..dea9a2a 100644
--- a/client/transfer.c
+++ b/client/transfer.c
@@ -143,11 +143,15 @@ static void obc_transfer_abort(struct obc_transfer *transfer)
 {
 	struct transfer_callback *callback = transfer->callback;
 
-	if (transfer->xfer == 0)
-		return;
+	if (transfer->xfer != 0) {
+		g_obex_cancel_transfer(transfer->xfer);
+		transfer->xfer = 0;
+	}
 
-	g_obex_cancel_transfer(transfer->xfer);
-	transfer->xfer = 0;
+	if (transfer->obex != NULL) {
+		g_obex_unref(transfer->obex);
+		transfer->obex = NULL;
+	}
 
 	if (callback) {
 		GError *err;
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH obexd v2 8/8] client: make sure callback does not match size
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
                   ` (6 preceding siblings ...)
  2012-02-21 13:57 ` [PATCH obexd v2 7/8] client: fix canceling queued transfers Mikel Astiz
@ 2012-02-21 13:57 ` Mikel Astiz
  2012-02-24  9:33 ` [PATCH obexd v2 0/8] Transfer cancelation Luiz Augusto von Dentz
  8 siblings, 0 replies; 17+ messages in thread
From: Mikel Astiz @ 2012-02-21 13:57 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Mikel Astiz

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Otherwise it can be interpreted as successfully finished, which has its
own code path.
---
 client/transfer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/client/transfer.c b/client/transfer.c
index dea9a2a..e749465 100644
--- a/client/transfer.c
+++ b/client/transfer.c
@@ -395,7 +395,7 @@ static void get_buf_xfer_progress(GObex *obex, GError *err, GObexPacket *rsp,
 							transfer, &err);
 	}
 
-	if (callback)
+	if (callback && transfer->transferred != transfer->size)
 		callback->func(transfer, transfer->transferred, err,
 							callback->data);
 }
@@ -439,7 +439,7 @@ static gboolean get_xfer_progress(const void *buf, gsize len,
 		transfer->filled -= w;
 	}
 
-	if (callback)
+	if (callback && transfer->transferred != transfer->size)
 		callback->func(transfer, transfer->transferred, NULL,
 							callback->data);
 
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers
  2012-02-21 13:57 ` [PATCH obexd v2 7/8] client: fix canceling queued transfers Mikel Astiz
@ 2012-02-22 11:41   ` Jaganath
  2012-02-22 11:59     ` Jaganath
  2012-02-26 17:26   ` Johan Hedberg
  1 sibling, 1 reply; 17+ messages in thread
From: Jaganath @ 2012-02-22 11:41 UTC (permalink / raw)
  To: Mikel Astiz, linux-bluetooth; +Cc: Mikel Astiz

Hi Mikel,

--------------------------------------------------
From: "Mikel Astiz" <mikel.astiz.oss@gmail.com>
Sent: Tuesday, February 21, 2012 7:27 PM
To: <linux-bluetooth@vger.kernel.org>
Cc: "Mikel Astiz" <mikel.astiz@bmw-carit.de>
Subject: [PATCH obexd v2 7/8] client: fix canceling queued transfers

> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> The Cancel() method in the D-Bus api should also abort queued transfers,
> which should just be removed from the queue.
> ---
> client/transfer.c |   12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/client/transfer.c b/client/transfer.c
> index b470a3a..dea9a2a 100644
> --- a/client/transfer.c
> +++ b/client/transfer.c
> @@ -143,11 +143,15 @@ static void obc_transfer_abort(struct obc_transfer 
> *transfer)
> {
>  struct transfer_callback *callback = transfer->callback;
>
> - if (transfer->xfer == 0)
> - return;
> + if (transfer->xfer != 0) {
> + g_obex_cancel_transfer(transfer->xfer);
> + transfer->xfer = 0;
> + }
>
> - g_obex_cancel_transfer(transfer->xfer);
> - transfer->xfer = 0;
> + if (transfer->obex != NULL) {
> + g_obex_unref(transfer->obex);
> + transfer->obex = NULL;
> + }

If transfer->obex is unrefed here then the queued ABORT packet will not be 
sent.
This will create problem with PTS which requires ABORT command before 
transport disconnection

>
>  if (callback) {
>  GError *err;
> -- 
> 1.7.6.5
>
> --

Regards
Jaganath
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers
  2012-02-22 11:41   ` Jaganath
@ 2012-02-22 11:59     ` Jaganath
  2012-02-22 12:15       ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 17+ messages in thread
From: Jaganath @ 2012-02-22 11:59 UTC (permalink / raw)
  To: Jaganath, Mikel Astiz, linux-bluetooth; +Cc: Mikel Astiz

Hi Mikel.

--------------------------------------------------
From: "Jaganath" <jaganath.k@samsung.com>
Sent: Wednesday, February 22, 2012 5:11 PM
To: "Mikel Astiz" <mikel.astiz.oss@gmail.com>; 
<linux-bluetooth@vger.kernel.org>
Cc: "Mikel Astiz" <mikel.astiz@bmw-carit.de>
Subject: Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers

> Hi Mikel,
>
> --------------------------------------------------
> From: "Mikel Astiz" <mikel.astiz.oss@gmail.com>
> Sent: Tuesday, February 21, 2012 7:27 PM
> To: <linux-bluetooth@vger.kernel.org>
> Cc: "Mikel Astiz" <mikel.astiz@bmw-carit.de>
> Subject: [PATCH obexd v2 7/8] client: fix canceling queued transfers
>
>> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>>
>> The Cancel() method in the D-Bus api should also abort queued transfers,
>> which should just be removed from the queue.
>> ---
>> client/transfer.c |   12 ++++++++----
>> 1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/client/transfer.c b/client/transfer.c
>> index b470a3a..dea9a2a 100644
>> --- a/client/transfer.c
>> +++ b/client/transfer.c
>> @@ -143,11 +143,15 @@ static void obc_transfer_abort(struct obc_transfer 
>> *transfer)
>> {
>>  struct transfer_callback *callback = transfer->callback;
>>
>> - if (transfer->xfer == 0)
>> - return;
>> + if (transfer->xfer != 0) {
>> + g_obex_cancel_transfer(transfer->xfer);
>> + transfer->xfer = 0;
>> + }
>>
>> - g_obex_cancel_transfer(transfer->xfer);
>> - transfer->xfer = 0;
>> + if (transfer->obex != NULL) {
>> + g_obex_unref(transfer->obex);
>> + transfer->obex = NULL;
>> + }
>
> If transfer->obex is unrefed here then the queued ABORT packet will not be 
> sent.
> This will create problem with PTS which requires ABORT command before 
> transport disconnection

Sorry. I think my comment is invalid since refcount will not be zero in this 
case.

>
>>
>>  if (callback) {
>>  GError *err;
>> -- 
>> 1.7.6.5
>>
>> --
>

Regards
Jaganath

>> To unsubscribe from this list: send the line "unsubscribe 
>> linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers
  2012-02-22 11:59     ` Jaganath
@ 2012-02-22 12:15       ` Luiz Augusto von Dentz
  2012-02-22 12:24         ` Jaganath
  0 siblings, 1 reply; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2012-02-22 12:15 UTC (permalink / raw)
  To: Jaganath; +Cc: Mikel Astiz, linux-bluetooth, Mikel Astiz

Hi Jaganath,

On Wed, Feb 22, 2012 at 1:59 PM, Jaganath <jaganath.k@samsung.com> wrote:
>> If transfer->obex is unrefed here then the queued ABORT packet will not be
>> sent.
>> This will create problem with PTS which requires ABORT command before
>> transport disconnection
>
>
> Sorry. I think my comment is invalid since refcount will not be zero in this
> case.

Yep, this is the transfer reference, btw what test are you referring
to? I don't remember having problem with PTS in case of OpenOBEX and
we didn't wait any abort to be sent, but perhaps that is because
OpenOBEX had the blocking write logic while gobex wait for POLL_OUT,
perhaps we should flush when disconnecting. Anyway PTS is pretty lame
since the abort request is useless if it is followed by a disconnect.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers
  2012-02-22 12:15       ` Luiz Augusto von Dentz
@ 2012-02-22 12:24         ` Jaganath
  2012-02-22 12:39           ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 17+ messages in thread
From: Jaganath @ 2012-02-22 12:24 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: Mikel Astiz, linux-bluetooth, Mikel Astiz

Hi Luiz,

--------------------------------------------------
From: "Luiz Augusto von Dentz" <luiz.dentz@gmail.com>
Sent: Wednesday, February 22, 2012 5:45 PM
To: "Jaganath" <jaganath.k@samsung.com>
Cc: "Mikel Astiz" <mikel.astiz.oss@gmail.com>; 
<linux-bluetooth@vger.kernel.org>; "Mikel Astiz" <mikel.astiz@bmw-carit.de>
Subject: Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers

> Hi Jaganath,
>
> On Wed, Feb 22, 2012 at 1:59 PM, Jaganath <jaganath.k@samsung.com> wrote:
>>> If transfer->obex is unrefed here then the queued ABORT packet will not 
>>> be
>>> sent.
>>> This will create problem with PTS which requires ABORT command before
>>> transport disconnection
>>
>>
>> Sorry. I think my comment is invalid since refcount will not be zero in 
>> this
>> case.
>
> Yep, this is the transfer reference, btw what test are you referring
> to? I don't remember having problem with PTS in case of OpenOBEX and
> we didn't wait any abort to be sent, but perhaps that is because
> OpenOBEX had the blocking write logic while gobex wait for POLL_OUT,

Correct. Because of POLL_OUT logic only it is not working. It was working 
earlier
with OpenOBEX.

> perhaps we should flush when disconnecting. Anyway PTS is pretty lame
> since the abort request is useless if it is followed by a disconnect.

I agree. But I think we should pass PTS test of qualification.

>
> -- 
> Luiz Augusto von Dentz
> --

Regards
Jaganath

> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers
  2012-02-22 12:24         ` Jaganath
@ 2012-02-22 12:39           ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2012-02-22 12:39 UTC (permalink / raw)
  To: Jaganath; +Cc: Mikel Astiz, linux-bluetooth, Mikel Astiz

Hi Jaganath,

On Wed, Feb 22, 2012 at 2:24 PM, Jaganath <jaganath.k@samsung.com> wrote:
>> Yep, this is the transfer reference, btw what test are you referring
>> to? I don't remember having problem with PTS in case of OpenOBEX and
>> we didn't wait any abort to be sent, but perhaps that is because
>> OpenOBEX had the blocking write logic while gobex wait for POLL_OUT,
>
>
> Correct. Because of POLL_OUT logic only it is not working. It was working
> earlier
> with OpenOBEX.

Ok, we should be able to fix it somehow, if we have something on
tx_queue we should perhaps just send it before disconnecting. But this
has nothing to do with this patch so lets avoid confusion here.

>> perhaps we should flush when disconnecting. Anyway PTS is pretty lame
>> since the abort request is useless if it is followed by a disconnect.
>
>
> I agree. But I think we should pass PTS test of qualification.

Yes, no questions about it.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH obexd v2 0/8] Transfer cancelation
  2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
                   ` (7 preceding siblings ...)
  2012-02-21 13:57 ` [PATCH obexd v2 8/8] client: make sure callback does not match size Mikel Astiz
@ 2012-02-24  9:33 ` Luiz Augusto von Dentz
  8 siblings, 0 replies; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2012-02-24  9:33 UTC (permalink / raw)
  To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz

Hi Mikel,

On Tue, Feb 21, 2012 at 3:57 PM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> This patch series proposes several fixes regarding the cancelation of transfers.
>
> The goal is that the Cancel method defined in the transfer's D-Bus API should work for any registered transfer, be it in progress or queued. Currently none of them seem to work properly.
>
> This second version is mostly a refacting of the previous proposal, according to the review from Luiz. The exceptions are patch 4/9 which has now been dropped, and patch 7/9, which now prevents from setting the callback twice.
>
> Mikel Astiz (8):
>  gobex: fix callback remove when canceling transfer
>  client: fix obc_session_get_buffer
>  client: fix cancel when no agent present
>  client: process transfer queue only if none active
>  client: terminate queued transfers properly
>  client: expose obc_transfer_set_callback
>  client: fix canceling queued transfers
>  client: make sure callback does not match size
>
>  client/session.c  |   45 ++++++++++++++++++++++++++++++++++++++-------
>  client/transfer.c |   37 ++++++++++++++++++-------------------
>  client/transfer.h |   10 ++++++----
>  gobex/gobex.c     |    3 +++
>  4 files changed, 65 insertions(+), 30 deletions(-)
>
> --
> 1.7.6.5

Ack

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH obexd v2 5/8] client: terminate queued transfers properly
  2012-02-21 13:57 ` [PATCH obexd v2 5/8] client: terminate queued transfers properly Mikel Astiz
@ 2012-02-26 17:23   ` Johan Hedberg
  0 siblings, 0 replies; 17+ messages in thread
From: Johan Hedberg @ 2012-02-26 17:23 UTC (permalink / raw)
  To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz

Hi Mikel,

On Tue, Feb 21, 2012, Mikel Astiz wrote:
> +static gint pending_transfer_cmptransfer(gconstpointer a, gconstpointer b)
> +{
> +	const struct pending_request *p = a;
> +	const struct obc_transfer *transfer = b;
> +
> +	return p->transfer < transfer ? -1 : (p->transfer > transfer ? 1 : 0);
> +}

Please rewrite this conditional statement into something more readable.
One has to spend much too much time trying to figure out what it does.
Since you're not using this for sorting but just for finding a specific
transfer you could just make it something like:

	if (p->transfer == transfer)
		return 0;

	return -1;

Johan

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers
  2012-02-21 13:57 ` [PATCH obexd v2 7/8] client: fix canceling queued transfers Mikel Astiz
  2012-02-22 11:41   ` Jaganath
@ 2012-02-26 17:26   ` Johan Hedberg
  1 sibling, 0 replies; 17+ messages in thread
From: Johan Hedberg @ 2012-02-26 17:26 UTC (permalink / raw)
  To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz

Hi Mikel,

On Tue, Feb 21, 2012, Mikel Astiz wrote:
> -	if (transfer->xfer == 0)
> -		return;
> +	if (transfer->xfer != 0) {
> +		g_obex_cancel_transfer(transfer->xfer);
> +		transfer->xfer = 0;
> +	}

Could you make the tests for a valid source ID > 0 instead of != 0. I
know it doesn't make much difference but it'd be consistent with that
we've tried to use elsewhere in obexd and bluez.

Btw, other than patches 5 and 7 where I had something to point out the
other ones have already been pushed upstream, so no need to resend them.

Johan

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2012-02-26 17:26 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-21 13:57 [PATCH obexd v2 0/8] Transfer cancelation Mikel Astiz
2012-02-21 13:57 ` [PATCH obexd v2 1/8] gobex: fix callback remove when canceling transfer Mikel Astiz
2012-02-21 13:57 ` [PATCH obexd v2 2/8] client: fix obc_session_get_buffer Mikel Astiz
2012-02-21 13:57 ` [PATCH obexd v2 3/8] client: fix cancel when no agent present Mikel Astiz
2012-02-21 13:57 ` [PATCH obexd v2 4/8] client: process transfer queue only if none active Mikel Astiz
2012-02-21 13:57 ` [PATCH obexd v2 5/8] client: terminate queued transfers properly Mikel Astiz
2012-02-26 17:23   ` Johan Hedberg
2012-02-21 13:57 ` [PATCH obexd v2 6/8] client: expose obc_transfer_set_callback Mikel Astiz
2012-02-21 13:57 ` [PATCH obexd v2 7/8] client: fix canceling queued transfers Mikel Astiz
2012-02-22 11:41   ` Jaganath
2012-02-22 11:59     ` Jaganath
2012-02-22 12:15       ` Luiz Augusto von Dentz
2012-02-22 12:24         ` Jaganath
2012-02-22 12:39           ` Luiz Augusto von Dentz
2012-02-26 17:26   ` Johan Hedberg
2012-02-21 13:57 ` [PATCH obexd v2 8/8] client: make sure callback does not match size Mikel Astiz
2012-02-24  9:33 ` [PATCH obexd v2 0/8] Transfer cancelation Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).