[PATCH obexd 1/2] gobex: flush tx_queue before disconnection

[PATCH obexd 1/2] gobex: flush tx_queue before disconnection

[Jaganath Kanakkassery]

This patch send the packets in tx_queue before the actual disconnection
If the operation is aborted it just sends ABORT and ignore the
rest of the packets in the queue.
---
 gobex/gobex.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gobex/gobex.c b/gobex/gobex.c
index bc76e57..496de46 100644
--- a/gobex/gobex.c
+++ b/gobex/gobex.c
@@ -1324,7 +1324,7 @@ GObex *g_obex_ref(GObex *obex)
 
 void g_obex_unref(GObex *obex)
 {
-	gboolean last_ref;
+	gboolean last_ref, ret;
 
 	last_ref = g_atomic_int_dec_and_test(&obex->ref_count);
 
@@ -1335,6 +1335,12 @@ void g_obex_unref(GObex *obex)
 
 	g_slist_free_full(obex->req_handlers, g_free);
 
+	do {
+		ret = write_data(obex->io, G_IO_OUT, obex);
+		if (obex->pending_req && obex->pending_req->cancelled)
+			break;
+	} while(ret);
+
 	g_queue_foreach(obex->tx_queue, (GFunc) pending_pkt_free, NULL);
 	g_queue_free(obex->tx_queue);
 
-- 
1.7.1

Re: [PATCH obexd 1/2] gobex: flush tx_queue before disconnection

[Johan Hedberg]

Hi Jaganath,

On Thu, Mar 08, 2012, Jaganath Kanakkassery wrote:
> @@ -1335,6 +1335,12 @@ void g_obex_unref(GObex *obex)
>  
>  	g_slist_free_full(obex->req_handlers, g_free);
>  
> +	do {
> +		ret = write_data(obex->io, G_IO_OUT, obex);
> +		if (obex->pending_req && obex->pending_req->cancelled)
> +			break;
> +	} while(ret);

This is not ok since we should only attempt writing to the transport if
G_IO_OUT is *really* set and not just fake it. Otherwise the call might
block which is not acceptable for the way gobex is designed (to be used
with a single async mainloop).

Johan

Re: [PATCH obexd 1/2] gobex: flush tx_queue before disconnection

[Jaganath]

Hi Johan,

--------------------------------------------------
From: "Johan Hedberg" <johan.hedberg@gmail.com>
Sent: Friday, March 09, 2012 5:12 AM
To: "Jaganath Kanakkassery" <jaganath.k@samsung.com>
Cc: <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH obexd 1/2] gobex: flush tx_queue before disconnection

> Hi Jaganath,
>
> On Thu, Mar 08, 2012, Jaganath Kanakkassery wrote:
>> @@ -1335,6 +1335,12 @@ void g_obex_unref(GObex *obex)
>>
>>  g_slist_free_full(obex->req_handlers, g_free);
>>
>> + do {
>> + ret = write_data(obex->io, G_IO_OUT, obex);
>> + if (obex->pending_req && obex->pending_req->cancelled)
>> + break;
>> + } while(ret);
>
> This is not ok since we should only attempt writing to the transport if
> G_IO_OUT is *really* set and not just fake it. Otherwise the call might
> block which is not acceptable for the way gobex is designed (to be used
> with a single async mainloop).

This actually fixes the issue abort req not sending if user cancels the
transfer because in user cancel scenario, ABORT req will be queued
but before G_IO_OUT happens the transport will be disconnected.
This is actually required to pass a pts test case because pts requires
ABORT before disconnection.

So I have two solutions after considering your and Luiz's opinion. '

1. In g_obex_send_internal() if the packet is ABORT we should send it
immediately using write_data(). Since ABORT packet size is small I think
it is ok to block the write.

2.  If user cancels transfer just send ABORT and disconnect transport only
after getting the response. But waiting for ABORT response is not good I 
feel.

Please let me know if you have better method.

Thanks,
Jaganath 

Re: [PATCH obexd 1/2] gobex: flush tx_queue before disconnection

[Johan Hedberg]

Hi Jaganath,

On Fri, Mar 09, 2012, Jaganath wrote:
> >On Thu, Mar 08, 2012, Jaganath Kanakkassery wrote:
> >>@@ -1335,6 +1335,12 @@ void g_obex_unref(GObex *obex)
> >>
> >> g_slist_free_full(obex->req_handlers, g_free);
> >>
> >>+ do {
> >>+ ret = write_data(obex->io, G_IO_OUT, obex);
> >>+ if (obex->pending_req && obex->pending_req->cancelled)
> >>+ break;
> >>+ } while(ret);
> >
> >This is not ok since we should only attempt writing to the transport if
> >G_IO_OUT is *really* set and not just fake it. Otherwise the call might
> >block which is not acceptable for the way gobex is designed (to be used
> >with a single async mainloop).
> 
> This actually fixes the issue abort req not sending if user cancels the
> transfer because in user cancel scenario, ABORT req will be queued
> but before G_IO_OUT happens the transport will be disconnected.
> This is actually required to pass a pts test case because pts requires
> ABORT before disconnection.

I fully understand that this needs to be fixed, it's just that the way
you're proposing is not a good way to do it.

> So I have two solutions after considering your and Luiz's opinion. '
> 
> 1. In g_obex_send_internal() if the packet is ABORT we should send it
> immediately using write_data(). Since ABORT packet size is small I think
> it is ok to block the write.

Nope, it's never ok to block on write.

> 2.  If user cancels transfer just send ABORT and disconnect transport only
> after getting the response. But waiting for ABORT response is not
> good I feel.

I think it *is* good to wait for the response. I've seen devices which
break if you disconnect the transport without waiting for OBEX
Disconnect command response and there could also be devices which
exhibit similar behavior for Abort.

I think this needs to be fixed on a higher layer than gobex as gobex
already provides a mechanism for waiting for the completion (or
cancellation) of a command.

Johan

Re: [PATCH obexd 1/2] gobex: flush tx_queue before disconnection

[Jaganath]

Hi Johan,

--------------------------------------------------
From: "Johan Hedberg" <johan.hedberg@gmail.com>
Sent: Friday, March 09, 2012 6:38 PM
To: "Jaganath" <jaganath.k@samsung.com>
Cc: <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH obexd 1/2] gobex: flush tx_queue before disconnection

> Hi Jaganath,
>
> On Fri, Mar 09, 2012, Jaganath wrote:
>> >On Thu, Mar 08, 2012, Jaganath Kanakkassery wrote:
>> >>@@ -1335,6 +1335,12 @@ void g_obex_unref(GObex *obex)
>> >>
>> >> g_slist_free_full(obex->req_handlers, g_free);
>> >>
>> >>+ do {
>> >>+ ret = write_data(obex->io, G_IO_OUT, obex);
>> >>+ if (obex->pending_req && obex->pending_req->cancelled)
>> >>+ break;
>> >>+ } while(ret);
>> >
>> >This is not ok since we should only attempt writing to the transport if
>> >G_IO_OUT is *really* set and not just fake it. Otherwise the call might
>> >block which is not acceptable for the way gobex is designed (to be used
>> >with a single async mainloop).
>>
>> This actually fixes the issue abort req not sending if user cancels the
>> transfer because in user cancel scenario, ABORT req will be queued
>> but before G_IO_OUT happens the transport will be disconnected.
>> This is actually required to pass a pts test case because pts requires
>> ABORT before disconnection.
>
> I fully understand that this needs to be fixed, it's just that the way
> you're proposing is not a good way to do it.
>
>> So I have two solutions after considering your and Luiz's opinion. '
>>
>> 1. In g_obex_send_internal() if the packet is ABORT we should send it
>> immediately using write_data(). Since ABORT packet size is small I think
>> it is ok to block the write.
>
> Nope, it's never ok to block on write.
>
>> 2.  If user cancels transfer just send ABORT and disconnect transport 
>> only
>> after getting the response. But waiting for ABORT response is not
>> good I feel.
>
> I think it *is* good to wait for the response. I've seen devices which
> break if you disconnect the transport without waiting for OBEX
> Disconnect command response and there could also be devices which
> exhibit similar behavior for Abort.

I have already sent a patch which wait for abort response before 
disconnection.
Subject:  [PATCH obexd] client: Fix ABORT command not sending when user
cancels the transfer.
Please review it and let me know your comments

> I think this needs to be fixed on a higher layer than gobex as gobex
> already provides a mechanism for waiting for the completion (or
> cancellation) of a command.

Thanks,
Jaganath

 

Re: [PATCH obexd 1/2] gobex: flush tx_queue before disconnection

[Luiz Augusto von Dentz]

Hi,

On Mon, Mar 12, 2012 at 7:42 AM, Jaganath <jaganath.k@samsung.com> wrote:
> I have already sent a patch which wait for abort response before
> disconnection.
> Subject:  [PATCH obexd] client: Fix ABORT command not sending when user
> cancels the transfer.
> Please review it and let me know your comments

Afaik it didn't invalidate the transfer id thus the callback and
user_data are still reachable so you might need to check again, also
it should no longer be possible to cancel using the same id too.

My proposal was actually to add g_obex_flush/g_obex_shutdown which can
be used similarly to g_io_channel_shutdown, although for this specific
problem it may not be the best solution due to stacks breaking if the
transport is disconnected while responding, in case of SRM there maybe
no response to wait and several packet are queued waiting POLL_OUT
while the application wants to disconnect.

Btw I don't think calling write_data will block since the io channel
should be non-blocking so we could have the same behavior as
g_io_channel_shutdown and return G_IO_STATUS_AGAIN if the packets
could not be flushed.

-- 
Luiz Augusto von Dentz