From: Mikel Astiz <[email protected]>
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
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
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
Hi Mikel,
On Tue, Feb 21, 2012 at 3:57 PM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> 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
Hi Jaganath,
On Wed, Feb 22, 2012 at 2:24 PM, Jaganath <[email protected]> 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
Hi Luiz,
--------------------------------------------------
From: "Luiz Augusto von Dentz" <[email protected]>
Sent: Wednesday, February 22, 2012 5:45 PM
To: "Jaganath" <[email protected]>
Cc: "Mikel Astiz" <[email protected]>;
<[email protected]>; "Mikel Astiz" <[email protected]>
Subject: Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers
> Hi Jaganath,
>
> On Wed, Feb 22, 2012 at 1:59 PM, Jaganath <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jaganath,
On Wed, Feb 22, 2012 at 1:59 PM, Jaganath <[email protected]> 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
Hi Mikel.
--------------------------------------------------
From: "Jaganath" <[email protected]>
Sent: Wednesday, February 22, 2012 5:11 PM
To: "Mikel Astiz" <[email protected]>;
<[email protected]>
Cc: "Mikel Astiz" <[email protected]>
Subject: Re: [PATCH obexd v2 7/8] client: fix canceling queued transfers
> Hi Mikel,
>
> --------------------------------------------------
> From: "Mikel Astiz" <[email protected]>
> Sent: Tuesday, February 21, 2012 7:27 PM
> To: <[email protected]>
> Cc: "Mikel Astiz" <[email protected]>
> Subject: [PATCH obexd v2 7/8] client: fix canceling queued transfers
>
>> From: Mikel Astiz <[email protected]>
>>
>> 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 [email protected]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mikel,
--------------------------------------------------
From: "Mikel Astiz" <[email protected]>
Sent: Tuesday, February 21, 2012 7:27 PM
To: <[email protected]>
Cc: "Mikel Astiz" <[email protected]>
Subject: [PATCH obexd v2 7/8] client: fix canceling queued transfers
> From: Mikel Astiz <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mikel Astiz <[email protected]>
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
From: Mikel Astiz <[email protected]>
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
From: Mikel Astiz <[email protected]>
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
From: Mikel Astiz <[email protected]>
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
From: Mikel Astiz <[email protected]>
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
From: Mikel Astiz <[email protected]>
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
From: Mikel Astiz <[email protected]>
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
From: Mikel Astiz <[email protected]>
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