[PATCH obexd] Fix handling asynchronous plugin reads

[PATCH obexd] Fix handling asynchronous plugin reads

[Slawomir Bochenski]

Calling OBEX_ResumeRequest() from handle_async_io() may result in direct
calling obex_event_cb() (this happens when obex_write_stream() will
deliver not enough bytes to fully fill OpenOBEX TX packet). In this case
set_io_watch will fail if handle_async_io() is called from
obex_object_set_io_flags(), because the watch is already installed.
Originally when code returns from OBEX_ResumeRequest(), handle_async_io()
returns FALSE which makes obex_object_set_io_flags() remove this watch.

This patch adds variable for tracking whether subsequent calls suspended
get request, causing obex_object_set_io_flags() remove the watch only
when the request is not suspended.
---
 src/obex-priv.h |    1 +
 src/obex.c      |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/src/obex-priv.h b/src/obex-priv.h
index d640ae0..74b42d3 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -49,6 +49,7 @@ struct obex_session {
 	struct obex_mime_type_driver *driver;
 	gboolean streaming;
 	gboolean headers_sent;
+	gboolean suspended;
 };
 
 int obex_session_start(GIOChannel *io, uint16_t tx_mtu, uint16_t rx_mtu,
diff --git a/src/obex.c b/src/obex.c
index 49a110d..29dea20 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -320,6 +320,7 @@ static void os_reset_session(struct obex_session *os)
 	os->size = OBJECT_SIZE_DELETE;
 	os->headers_sent = FALSE;
 	os->streaming = FALSE;
+	os->suspended = FALSE;
 }
 
 static void obex_session_free(struct obex_session *os)
@@ -747,6 +748,8 @@ static gboolean handle_async_io(void *object, int flags, int err,
 		ret = obex_read_stream(os, os->obex, os->obj);
 
 proceed:
+	os->suspended = FALSE;
+
 	if (ret == -EAGAIN) {
 		return TRUE;
 	} else if (ret < 0) {
@@ -756,7 +759,7 @@ proceed:
 		OBEX_ResumeRequest(os->obex);
 	}
 
-	return FALSE;
+	return os->suspended;
 }
 
 static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
@@ -1355,6 +1358,7 @@ static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
 		err = obex_write_stream(os, obex, obj);
 		if (err == -EAGAIN) {
 			OBEX_SuspendRequest(obex, obj);
+			os->suspended = TRUE;
 			os->obj = obj;
 			os->driver->set_io_watch(os->object, handle_async_io,
 									os);
-- 
1.7.4.1

Re: [PATCH obexd] Fix handling asynchronous plugin reads

[Johan Hedberg]

Hi Slawek,

On Tue, Jul 19, 2011, Slawomir Bochenski wrote:
> @@ -49,6 +49,7 @@ struct obex_session {
>  	struct obex_mime_type_driver *driver;
>  	gboolean streaming;
>  	gboolean headers_sent;
> +	gboolean suspended;

There's something redundant or unintuitive about the streaming vs.
suspended variable naming. I would expect suspended to be the logical
inverse of streaming, i.e. when streaming == TRUE then suspended ==
FALSE and vice versa. So one of them shouldn't be needed. And if this is
not their meaning then they should probably be renamed more intuitively.
Btw, is any of this relevant once the migration to gobex has been done?
I think this is something important to keep in mind whenever you do any
changes due to some quirkiness of OpenOBEX.

Johan

Re: [PATCH obexd] Fix handling asynchronous plugin reads

[Slawomir Bochenski]

Hello!

On Tue, Jul 19, 2011 at 1:19 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Slawek,
>
> On Tue, Jul 19, 2011, Slawomir Bochenski wrote:
>> @@ -49,6 +49,7 @@ struct obex_session {
>>       struct obex_mime_type_driver *driver;
>>       gboolean streaming;
>>       gboolean headers_sent;
>> +     gboolean suspended;
>
> There's something redundant or unintuitive about the streaming vs.
> suspended variable naming. I would expect suspended to be the logical
> inverse of streaming, i.e. when streaming == TRUE then suspended ==
> FALSE and vice versa. So one of them shouldn't be needed. And if this is
> not their meaning then they should probably be renamed more intuitively.
> Btw, is any of this relevant once the migration to gobex has been done?
> I think this is something important to keep in mind whenever you do any
> changes due to some quirkiness of OpenOBEX.
>
> Johan
>

The issue is still present in current upstream tree and seems to
prevent MAP from working.

If it goes about names that would be one of streaming_started instead
of streaming *or* streaming_suspended instead of suspended. First one
is used for indication whether streaming was actually started (which
is done once) and the second one is for checking whether chain of
called functions suspended currently streamed body.

-- 
Slawomir Bochenski