Various GET functionality fixes (obsoletes: Renewed new missed calls handling)

Various GET functionality fixes (obsoletes: Renewed new missed calls handling)

[Slawomir Bochenski]

These make the previous patch set I've sent obsolete.

Fist two patches are a clean-up of some code doing nothing.

Third patch is for proper (that is, the one required for PBAP and MAP)
headers order. Here solution for sending application parameters header
first is done inside obex_write_stream() (as suggested by Luiz), making
it work automatically (no changes to pbap coded needed).

The last patch offers a fix for a cases when read woken by
obex_object_set_io_flags() would not deliver enough data to fill OBEX
MTU, in which case asynchronous functionality was broken.

[PATCH 1/4] Revert API changes to mime driver read function

[Slawomir Bochenski]

This reverts parts of commit e0b3283e20ba885018010a6a8ae49b7c313958e8.  API
changes introduced before were meant to guarantee required PBAP functionality
of sending application parameters header in first packet of multi-packet
response.

However OBEX_FL_FIT_ONE_PACKET does not serve this purpose - it is for making
sure that all headers will fit fully into one packet (thus FIT_ONE_PACKET),
returning error from OBEX_ObjectAddHeader() when this is not the case.

Starting the body header streaming adds body header immediately to the outgoing
queue, waiting for more data. Any attempts to OBEX_ObjectAddHeader() different
than those with hi == OBEX_HDR_DATA and OBEX_FL_STREAM_DATA present in flags,
will add new header after currently streamed body and it will be processed
after ending streaming with OBEX_FL_STREAM_DATAEND.  Also in this case data is
memcpy'd, so no reason for any additional write_offset counting.
---
 plugins/filesystem.c    |   16 +++-------------
 plugins/irmc.c          |    6 +-----
 plugins/mas.c           |    4 +---
 plugins/pbap.c          |   19 +++----------------
 plugins/syncevolution.c |    6 +-----
 src/mimetype.h          |    3 +--
 src/obex-priv.h         |    1 -
 src/obex.c              |   15 +++++----------
 8 files changed, 15 insertions(+), 55 deletions(-)

diff --git a/plugins/filesystem.c b/plugins/filesystem.c
index 7bfe673..b4ff556 100644
--- a/plugins/filesystem.c
+++ b/plugins/filesystem.c
@@ -210,7 +210,7 @@ static int filesystem_close(void *object)
 }
 
 static ssize_t filesystem_read(void *object, void *buf, size_t count,
-					uint8_t *hi, unsigned int *flags)
+								uint8_t *hi)
 {
 	ssize_t ret;
 
@@ -218,9 +218,6 @@ static ssize_t filesystem_read(void *object, void *buf, size_t count,
 	if (ret < 0)
 		return -errno;
 
-	if (flags)
-		*flags = 0;
-
 	*hi = OBEX_HDR_BODY;
 
 	return ret;
@@ -502,24 +499,17 @@ ssize_t string_read(void *object, void *buf, size_t count)
 	return len;
 }
 
-static ssize_t folder_read(void *object, void *buf, size_t count,
-					uint8_t *hi, unsigned int *flags)
+static ssize_t folder_read(void *object, void *buf, size_t count, uint8_t *hi)
 {
-	if (flags)
-		*flags = 0;
-
 	*hi = OBEX_HDR_BODY;
 	return string_read(object, buf, count);
 }
 
 static ssize_t capability_read(void *object, void *buf, size_t count,
-					uint8_t *hi, unsigned int *flags)
+								uint8_t *hi)
 {
 	struct capability_object *obj = object;
 
-	if (flags)
-		*flags = 0;
-
 	*hi = OBEX_HDR_BODY;
 
 	if (obj->buffer)
diff --git a/plugins/irmc.c b/plugins/irmc.c
index fd233f7..cd7f386 100644
--- a/plugins/irmc.c
+++ b/plugins/irmc.c
@@ -463,8 +463,7 @@ static int irmc_close(void *object)
 	return 0;
 }
 
-static ssize_t irmc_read(void *object, void *buf, size_t count, uint8_t *hi,
-							unsigned int *flags)
+static ssize_t irmc_read(void *object, void *buf, size_t count, uint8_t *hi)
 {
 	struct irmc_session *irmc = object;
 	int len;
@@ -473,9 +472,6 @@ static ssize_t irmc_read(void *object, void *buf, size_t count, uint8_t *hi,
 	if (!irmc->buffer)
                 return -EAGAIN;
 
-	if (flags)
-		*flags = 0;
-
 	*hi = OBEX_HDR_BODY;
 	len = string_read(irmc->buffer, buf, count);
 	DBG("returning %d bytes", len);
diff --git a/plugins/mas.c b/plugins/mas.c
index fb1b13a..d13625c 100644
--- a/plugins/mas.c
+++ b/plugins/mas.c
@@ -225,13 +225,11 @@ static ssize_t any_write(void *object, const void *buf, size_t count)
 	return count;
 }
 
-static ssize_t any_read(void *obj, void *buf, size_t count,
-				uint8_t *hi, unsigned int *flags)
+static ssize_t any_read(void *obj, void *buf, size_t count, uint8_t *hi)
 {
 	DBG("");
 
 	*hi = OBEX_HDR_BODY;
-	*flags = 0;
 
 	return 0;
 }
diff --git a/plugins/pbap.c b/plugins/pbap.c
index c4df37f..0356ae7 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -967,7 +967,7 @@ static ssize_t array_read(GByteArray *array, void *buf, size_t count)
 }
 
 static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
-					uint8_t *hi, unsigned int *flags)
+								uint8_t *hi)
 {
 	struct pbap_object *obj = object;
 	struct pbap_session *pbap = obj->session;
@@ -982,22 +982,15 @@ static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
 	if (pbap->params->maxlistcount == 0) {
 		/* PhoneBookSize */
 		*hi = OBEX_HDR_APPARAM;
-		if (flags)
-			*flags = 0;
 		return array_read(obj->aparams, buf, count);
 	} else if (obj->firstpacket) {
 		/* NewMissedCalls */
 		*hi = OBEX_HDR_APPARAM;
 		obj->firstpacket = FALSE;
-		if (flags)
-			*flags = OBEX_FL_FIT_ONE_PACKET;
 		return array_read(obj->aparams, buf, count);
 	} else {
 		/* Stream data */
 		*hi = OBEX_HDR_BODY;
-		if (flags)
-			*flags = 0;
-
 		len = string_read(obj->buffer, buf, count);
 		if (len == 0 && !obj->lastpart) {
 			/* in case when buffer is empty and we know that more
@@ -1016,7 +1009,7 @@ static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
 }
 
 static ssize_t vobject_list_read(void *object, void *buf, size_t count,
-					uint8_t *hi, unsigned int *flags)
+								uint8_t *hi)
 {
 	struct pbap_object *obj = object;
 	struct pbap_session *pbap = obj->session;
@@ -1028,9 +1021,6 @@ static ssize_t vobject_list_read(void *object, void *buf, size_t count,
 	if (!pbap->cache.valid)
 		return -EAGAIN;
 
-	if (flags)
-		*flags = 0;
-
 	if (pbap->params->maxlistcount == 0) {
 		*hi = OBEX_HDR_APPARAM;
 		return array_read(obj->aparams, buf, count);
@@ -1041,7 +1031,7 @@ static ssize_t vobject_list_read(void *object, void *buf, size_t count,
 }
 
 static ssize_t vobject_vcard_read(void *object, void *buf, size_t count,
-					uint8_t *hi, unsigned int *flags)
+								uint8_t *hi)
 {
 	struct pbap_object *obj = object;
 
@@ -1050,9 +1040,6 @@ static ssize_t vobject_vcard_read(void *object, void *buf, size_t count,
 	if (!obj->buffer)
 		return -EAGAIN;
 
-	if (flags)
-		*flags = 0;
-
 	*hi = OBEX_HDR_BODY;
 	return string_read(obj->buffer, buf, count);
 }
diff --git a/plugins/syncevolution.c b/plugins/syncevolution.c
index 0575ab1..ea3eb7a 100644
--- a/plugins/syncevolution.c
+++ b/plugins/syncevolution.c
@@ -331,8 +331,7 @@ done:
 	return 0;
 }
 
-static ssize_t synce_read(void *object, void *buf, size_t count,
-					uint8_t *hi, unsigned int *flags)
+static ssize_t synce_read(void *object, void *buf, size_t count, uint8_t *hi)
 {
 	struct synce_context *context = object;
 	DBusConnection *conn;
@@ -343,9 +342,6 @@ static ssize_t synce_read(void *object, void *buf, size_t count,
 	gboolean authenticate;
 	DBusPendingCall *call;
 
-	if (flags)
-		*flags = 0;
-
 	if (context->buffer) {
 		*hi = OBEX_HDR_BODY;
 		return string_read(context->buffer, buf, count);
diff --git a/src/mimetype.h b/src/mimetype.h
index 5bf741c..8472684 100644
--- a/src/mimetype.h
+++ b/src/mimetype.h
@@ -33,8 +33,7 @@ struct obex_mime_type_driver {
 	void *(*open) (const char *name, int oflag, mode_t mode,
 			void *driver_data, size_t *size, int *err);
 	int (*close) (void *object);
-	ssize_t (*read) (void *object, void *buf, size_t count, uint8_t *hi,
-							unsigned int *flags);
+	ssize_t (*read) (void *object, void *buf, size_t count, uint8_t *hi);
 	ssize_t (*write) (void *object, const void *buf, size_t count);
 	int (*flush) (void *object);
 	int (*remove) (const char *name);
diff --git a/src/obex-priv.h b/src/obex-priv.h
index 2a22f38..0806a56 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -46,7 +46,6 @@ struct obex_session {
 	obex_object_t *obj;
 	struct obex_mime_type_driver *driver;
 	gboolean finished;
-	uint16_t write_offset;
 };
 
 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 caba2fb..643b942 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -643,13 +643,10 @@ static int obex_write_stream(struct obex_session *os,
 
 		len = MIN(os->size - os->offset, os->tx_mtu);
 		ptr = os->buf + os->offset;
-		flags = 0;
 		goto add_header;
 	}
 
-	ptr = os->buf + os->write_offset;
-	len = os->driver->read(os->object, ptr, os->tx_mtu - os->write_offset,
-								&hi, &flags);
+	len = os->driver->read(os->object, os->buf, os->tx_mtu, &hi);
 	if (len < 0) {
 		error("read(): %s (%zd)", strerror(-len), -len);
 		if (len == -EAGAIN)
@@ -662,15 +659,18 @@ static int obex_write_stream(struct obex_session *os,
 		return len;
 	}
 
+	ptr = os->buf;
+
 add_header:
 
 	hd.bs = ptr;
 
 	switch (hi) {
 	case OBEX_HDR_BODY:
-		flags |= len ? OBEX_FL_STREAM_DATA : OBEX_FL_STREAM_DATAEND;
+		flags = len ? OBEX_FL_STREAM_DATA : OBEX_FL_STREAM_DATAEND;
 		break;
 	case OBEX_HDR_APPARAM:
+		flags =  0;
 		break;
 	default:
 		error("read(): unkown header type %u", hi);
@@ -684,11 +684,6 @@ add_header:
 		os->buf = NULL;
 	}
 
-	if (flags & OBEX_FL_FIT_ONE_PACKET)
-		os->write_offset += len;
-	else
-		os->write_offset = 0;
-
 	os->offset += len;
 
 	return 0;
-- 
1.7.4.1

[PATCH 2/4] Remove redundant code

[Slawomir Bochenski]

This removes outdated (no longer used) code from obex_write_stream:

1) Support for sending data injected directly to obj->buf. This was used
for implementing folder listing in commit:
c42eff92a9c2c177f788dd1ec429250e64f69a78.
2) os->finished flag originally used for supporting asynchronous code in
PBAP, introduced in commit:
1a5025349df3dc6134db62afdcd048c00f876b27
---
 src/obex-priv.h |    1 -
 src/obex.c      |   20 +++-----------------
 2 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/src/obex-priv.h b/src/obex-priv.h
index 0806a56..0caa0dd 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -45,7 +45,6 @@ struct obex_session {
 	obex_t *obex;
 	obex_object_t *obj;
 	struct obex_mime_type_driver *driver;
-	gboolean finished;
 };
 
 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 643b942..e6585ca 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -310,7 +310,6 @@ static void os_reset_session(struct obex_session *os)
 	os->pending = 0;
 	os->offset = 0;
 	os->size = OBJECT_SIZE_DELETE;
-	os->finished = 0;
 }
 
 static void obex_session_free(struct obex_session *os)
@@ -625,7 +624,6 @@ static int obex_write_stream(struct obex_session *os,
 			obex_t *obex, obex_object_t *obj)
 {
 	obex_headerdata_t hd;
-	uint8_t *ptr;
 	ssize_t len;
 	unsigned int flags;
 	uint8_t hi;
@@ -637,14 +635,8 @@ static int obex_write_stream(struct obex_session *os,
 	if (os->aborted)
 		return -EPERM;
 
-	if (os->object == NULL) {
-		if (os->buf == NULL && os->finished == FALSE)
-			return -EIO;
-
-		len = MIN(os->size - os->offset, os->tx_mtu);
-		ptr = os->buf + os->offset;
-		goto add_header;
-	}
+	if (os->object == NULL)
+		return -EIO;
 
 	len = os->driver->read(os->object, os->buf, os->tx_mtu, &hi);
 	if (len < 0) {
@@ -659,11 +651,7 @@ static int obex_write_stream(struct obex_session *os,
 		return len;
 	}
 
-	ptr = os->buf;
-
-add_header:
-
-	hd.bs = ptr;
+	hd.bs = os->buf;
 
 	switch (hi) {
 	case OBEX_HDR_BODY:
@@ -684,8 +672,6 @@ add_header:
 		os->buf = NULL;
 	}
 
-	os->offset += len;
-
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 3/4] Modify obex_write_stream logic

[Slawomir Bochenski]

PBAP and MAP profiles present a requirement that in case of multi-packet
responses, application parameter header shall be send in the first
packet.

Starting body streaming in OpenOBEX adds body header immediately and
queues any other added header for sending after the streaming is
finished. In order to get desired behaviour streaming mode should be
started after sending all other headers.

This patch modifies obex_write_stream() to do this. 'stream' argument to
service driver get() function is removed, as it is no longer needed -
streaming is started automatically when getting first portion of body
data.

Also previously when the 'stream' argument was set to false, it was only
possible to add single header, as there was no loop nor appropriate
suspending involved (which is required in order to not finish
non-streaming request immediately).

Now suspending in non-streaming case is achieved by adding OBEX_HDR_EMPTY
with OBEX_FL_SUSPEND flag set, so OpenOBEX will send whatever data is
available and suspend after that.
---
 plugins/ftp.c           |    6 +--
 plugins/ftp.h           |    3 +-
 plugins/irmc.c          |    3 +-
 plugins/mas.c           |    5 +--
 plugins/opp.c           |    6 +--
 plugins/pbap.c          |    7 +---
 plugins/pcsuite.c       |    4 +-
 plugins/syncevolution.c |    5 +--
 src/obex-priv.h         |    1 +
 src/obex.c              |  100 ++++++++++++++++++++++++++++++-----------------
 src/service.h           |    2 +-
 11 files changed, 75 insertions(+), 67 deletions(-)

diff --git a/plugins/ftp.c b/plugins/ftp.c
index 79223bf..4962326 100644
--- a/plugins/ftp.c
+++ b/plugins/ftp.c
@@ -213,8 +213,7 @@ void *ftp_connect(struct obex_session *os, int *err)
 	return ftp;
 }
 
-int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
-							void *user_data)
+int ftp_get(struct obex_session *os, obex_object_t *obj, void *user_data)
 {
 	struct ftp_session *ftp = user_data;
 	const char *type = obex_get_type(os);
@@ -229,9 +228,6 @@ int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
 	if (ret < 0)
 		return ret;
 
-	if (stream)
-		*stream = TRUE;
-
 	return 0;
 }
 
diff --git a/plugins/ftp.h b/plugins/ftp.h
index 2374125..d929cde 100644
--- a/plugins/ftp.h
+++ b/plugins/ftp.h
@@ -22,8 +22,7 @@
  */
 
 void *ftp_connect(struct obex_session *os, int *err);
-int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
-							void *user_data);
+int ftp_get(struct obex_session *os, obex_object_t *obj, void *user_data);
 int ftp_chkput(struct obex_session *os, void *user_data);
 int ftp_put(struct obex_session *os, obex_object_t *obj, void *user_data);
 int ftp_setpath(struct obex_session *os, obex_object_t *obj, void *user_data);
diff --git a/plugins/irmc.c b/plugins/irmc.c
index cd7f386..cc0b9db 100644
--- a/plugins/irmc.c
+++ b/plugins/irmc.c
@@ -233,7 +233,7 @@ static void *irmc_connect(struct obex_session *os, int *err)
 }
 
 static int irmc_get(struct obex_session *os, obex_object_t *obj,
-		gboolean *stream, void *user_data)
+							void *user_data)
 {
 	struct irmc_session *irmc = user_data;
 	const char *type = obex_get_type(os);
@@ -244,7 +244,6 @@ static int irmc_get(struct obex_session *os, obex_object_t *obj,
 	DBG("name %s type %s irmc %p", name, type ? type : "NA", irmc);
 
 	path = g_strdup(name);
-	*stream = TRUE;
 
 	ret = obex_get_stream_start(os, path);
 
diff --git a/plugins/mas.c b/plugins/mas.c
index d13625c..08e47a2 100644
--- a/plugins/mas.c
+++ b/plugins/mas.c
@@ -134,8 +134,7 @@ static void mas_disconnect(struct obex_session *os, void *user_data)
 	mas_clean(mas);
 }
 
-static int mas_get(struct obex_session *os, obex_object_t *obj,
-					gboolean *stream, void *user_data)
+static int mas_get(struct obex_session *os, obex_object_t *obj, void *user_data)
 {
 	struct mas_session *mas = user_data;
 	const char *type = obex_get_type(os);
@@ -148,8 +147,6 @@ static int mas_get(struct obex_session *os, obex_object_t *obj,
 	if (type == NULL)
 		return -EBADR;
 
-	*stream = FALSE;
-
 	ret = obex_get_stream_start(os, name);
 	if (ret < 0)
 		goto failed;
diff --git a/plugins/opp.c b/plugins/opp.c
index 17c4356..4f0ed08 100644
--- a/plugins/opp.c
+++ b/plugins/opp.c
@@ -170,8 +170,7 @@ static int opp_put(struct obex_session *os, obex_object_t *obj,
 	return 0;
 }
 
-static int opp_get(struct obex_session *os, obex_object_t *obj,
-			gboolean *stream, void *user_data)
+static int opp_get(struct obex_session *os, obex_object_t *obj, void *user_data)
 {
 	const char *type;
 
@@ -190,9 +189,6 @@ static int opp_get(struct obex_session *os, obex_object_t *obj,
 	} else
 		return -EPERM;
 
-	if (stream)
-		*stream = TRUE;
-
 	return 0;
 }
 
diff --git a/plugins/pbap.c b/plugins/pbap.c
index 0356ae7..a5b0117 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -625,7 +625,7 @@ static void *pbap_connect(struct obex_session *os, int *err)
 }
 
 static int pbap_get(struct obex_session *os, obex_object_t *obj,
-					gboolean *stream, void *user_data)
+							void *user_data)
 {
 	struct pbap_session *pbap = user_data;
 	const char *type = obex_get_type(os);
@@ -662,8 +662,6 @@ static int pbap_get(struct obex_session *os, obex_object_t *obj,
 			path = g_strdup(name);
 		else
 			path = g_build_filename("/", name, NULL);
-
-		*stream = (params->maxlistcount == 0 ? FALSE : TRUE);
 	} else if (strcmp(type, VCARDLISTING_TYPE) == 0) {
 		/* Always relative */
 		if (!name || strlen(name) == 0)
@@ -672,12 +670,9 @@ static int pbap_get(struct obex_session *os, obex_object_t *obj,
 		else
 			/* Current folder + relative path */
 			path = g_build_filename(pbap->folder, name, NULL);
-
-		*stream = (params->maxlistcount == 0 ? FALSE : TRUE);
 	} else if (strcmp(type, VCARDENTRY_TYPE) == 0) {
 		/* File name only */
 		path = g_strdup(name);
-		*stream = TRUE;
 	} else
 		return -EBADR;
 
diff --git a/plugins/pcsuite.c b/plugins/pcsuite.c
index 9cf65c9..318e186 100644
--- a/plugins/pcsuite.c
+++ b/plugins/pcsuite.c
@@ -181,13 +181,13 @@ fail:
 }
 
 static int pcsuite_get(struct obex_session *os, obex_object_t *obj,
-					gboolean *stream, void *user_data)
+							void *user_data)
 {
 	struct pcsuite_session *pcsuite = user_data;
 
 	DBG("%p", pcsuite);
 
-	return ftp_get(os, obj, stream, pcsuite->ftp);
+	return ftp_get(os, obj, pcsuite->ftp);
 }
 
 static int pcsuite_chkput(struct obex_session *os, void *user_data)
diff --git a/plugins/syncevolution.c b/plugins/syncevolution.c
index ea3eb7a..77c1bd6 100644
--- a/plugins/syncevolution.c
+++ b/plugins/syncevolution.c
@@ -250,11 +250,8 @@ static int synce_put(struct obex_session *os, obex_object_t *obj,
 }
 
 static int synce_get(struct obex_session *os, obex_object_t *obj,
-					gboolean *stream, void *user_data)
+							void *user_data)
 {
-	if (stream)
-		*stream = TRUE;
-
 	return obex_get_stream_start(os, NULL);
 }
 
diff --git a/src/obex-priv.h b/src/obex-priv.h
index 0caa0dd..8c722dc 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -45,6 +45,7 @@ struct obex_session {
 	obex_t *obex;
 	obex_object_t *obj;
 	struct obex_mime_type_driver *driver;
+	gboolean streaming;
 };
 
 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 e6585ca..659f39b 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -310,6 +310,7 @@ static void os_reset_session(struct obex_session *os)
 	os->pending = 0;
 	os->offset = 0;
 	os->size = OBJECT_SIZE_DELETE;
+	os->streaming = FALSE;
 }
 
 static void obex_session_free(struct obex_session *os)
@@ -638,43 +639,78 @@ static int obex_write_stream(struct obex_session *os,
 	if (os->object == NULL)
 		return -EIO;
 
-	len = os->driver->read(os->object, os->buf, os->tx_mtu, &hi);
-	if (len < 0) {
-		error("read(): %s (%zd)", strerror(-len), -len);
-		if (len == -EAGAIN)
+	do {
+		len = os->driver->read(os->object, os->buf, os->tx_mtu, &hi);
+		if (len < 0) {
+			error("read(): %s (%zd)", strerror(-len), -len);
+			if (len == -EAGAIN)
+				return len;
+			else if (len == -ENOSTR)
+				return 0;
+
+			g_free(os->buf);
+			os->buf = NULL;
 			return len;
-		else if (len == -ENOSTR)
+		}
+
+		if (len == 0) {
+			if (os->streaming) {
+				hd.bs = NULL;
+				OBEX_ObjectAddHeader(obex, obj, hi, hd, 0,
+							OBEX_FL_STREAM_DATAEND);
+			}
+			g_free(os->buf);
+			os->buf = NULL;
 			return 0;
+		}
 
-		g_free(os->buf);
-		os->buf = NULL;
-		return len;
-	}
+		if (!os->streaming && hi == OBEX_HDR_BODY) {
+			DBG("Starting streaming");
+			hd.bs = NULL;
+			OBEX_ObjectAddHeader(obex, obj, hi, hd, 0,
+						OBEX_FL_STREAM_START);
+			os->streaming = TRUE;
+		}
 
-	hd.bs = os->buf;
+		hd.bs = os->buf;
 
-	switch (hi) {
-	case OBEX_HDR_BODY:
-		flags = len ? OBEX_FL_STREAM_DATA : OBEX_FL_STREAM_DATAEND;
-		break;
-	case OBEX_HDR_APPARAM:
-		flags =  0;
-		break;
-	default:
-		error("read(): unkown header type %u", hi);
-		return -EIO;
-	}
+		switch (hi) {
+		case OBEX_HDR_BODY:
+			flags = OBEX_FL_STREAM_DATA;
+			break;
+		case OBEX_HDR_APPARAM:
+			flags = 0;
+			break;
+		default:
+			error("read(): unkown header type %u", hi);
+			return -EIO;
+		}
 
-	OBEX_ObjectAddHeader(obex, obj, hi, hd, len, flags);
+		OBEX_ObjectAddHeader(obex, obj, hi, hd, len, flags);
 
-	if (len == 0) {
-		g_free(os->buf);
-		os->buf = NULL;
-	}
+	} while (!os->streaming);
 
 	return 0;
 }
 
+static void suspend_stream(struct obex_session *os, obex_t *obex,
+						obex_object_t *obj)
+{
+	obex_headerdata_t hd;
+
+	if (os->streaming) {
+		OBEX_SuspendRequest(obex, obj);
+		return;
+	}
+
+	hd.bs = NULL;
+	OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_EMPTY, hd, 0, OBEX_FL_SUSPEND);
+	/* If there were no headers in the queue after the one with
+	 * the suspend flag set, OpenOBEX would finalize the request.
+	 */
+	OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_EMPTY, hd, 0, 0);
+}
+
 static gboolean handle_async_io(void *object, int flags, int err,
 						void *user_data)
 {
@@ -704,7 +740,6 @@ proceed:
 static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
 {
 	obex_headerdata_t hd;
-	gboolean stream;
 	unsigned int hlen;
 	uint8_t hi;
 	int err;
@@ -781,7 +816,7 @@ static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
 		return;
 	}
 
-	err = os->service->get(os, obj, &stream, os->service_data);
+	err = os->service->get(os, obj, os->service_data);
 
 	if (err < 0)
 		goto done;
@@ -800,16 +835,9 @@ static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
 		goto done;
 	}
 
-	if (stream)
-		/* Standard data stream */
-		OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_BODY, hd, 0,
-						OBEX_FL_STREAM_START);
-
-	/* Try to write to stream and suspend the stream immediately
-	 * if no data available to send. */
 	err = obex_write_stream(os, obex, obj);
 	if (err == -EAGAIN) {
-		OBEX_SuspendRequest(obex, obj);
+		suspend_stream(os, obex, obj);
 		os->obj = obj;
 		os->driver->set_io_watch(os->object, handle_async_io, os);
 		return;
diff --git a/src/service.h b/src/service.h
index a844885..4d9d683 100644
--- a/src/service.h
+++ b/src/service.h
@@ -33,7 +33,7 @@ struct obex_service_driver {
 	void *(*connect) (struct obex_session *os, int *err);
 	void (*progress) (struct obex_session *os, void *user_data);
 	int (*get) (struct obex_session *os, obex_object_t *obj,
-			gboolean *stream, void *user_data);
+							void *user_data);
 	int (*put) (struct obex_session *os, obex_object_t *obj,
 			void *user_data);
 	int (*chkput) (struct obex_session *os, void *user_data);
-- 
1.7.4.1

[PATCH 4/4] 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
stream, causing obex_object_set_io_flags() remove the watch only when the
streaming is still running (i.e. not suspended).
---
 src/obex-priv.h |    1 +
 src/obex.c      |   15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/obex-priv.h b/src/obex-priv.h
index 8c722dc..54ec991 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -46,6 +46,7 @@ struct obex_session {
 	obex_object_t *obj;
 	struct obex_mime_type_driver *driver;
 	gboolean streaming;
+	gboolean stream_running;
 };
 
 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 659f39b..5f437c7 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -311,6 +311,7 @@ static void os_reset_session(struct obex_session *os)
 	os->offset = 0;
 	os->size = OBJECT_SIZE_DELETE;
 	os->streaming = FALSE;
+	os->stream_running = FALSE;
 }
 
 static void obex_session_free(struct obex_session *os)
@@ -670,6 +671,7 @@ static int obex_write_stream(struct obex_session *os,
 			OBEX_ObjectAddHeader(obex, obj, hi, hd, 0,
 						OBEX_FL_STREAM_START);
 			os->streaming = TRUE;
+			os->stream_running = TRUE;
 		}
 
 		hd.bs = os->buf;
@@ -698,6 +700,8 @@ static void suspend_stream(struct obex_session *os, obex_t *obex,
 {
 	obex_headerdata_t hd;
 
+	os->stream_running = FALSE;
+
 	if (os->streaming) {
 		OBEX_SuspendRequest(obex, obj);
 		return;
@@ -731,10 +735,15 @@ proceed:
 	if (ret < 0) {
 		os_set_response(os->obj, err);
 		OBEX_CancelRequest(os->obex, TRUE);
-	} else
+	} else {
+		os->stream_running = TRUE;
 		OBEX_ResumeRequest(os->obex);
+	}
 
-	return FALSE;
+	if (os->stream_running)
+		return FALSE;
+	else
+		return TRUE;
 }
 
 static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
@@ -1225,7 +1234,7 @@ static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
 	case OBEX_EV_STREAMEMPTY:
 		err = obex_write_stream(os, obex, obj);
 		if (err == -EAGAIN) {
-			OBEX_SuspendRequest(obex, obj);
+			suspend_stream(os, obex, obj);
 			os->obj = obj;
 			os->driver->set_io_watch(os->object, handle_async_io,
 									os);
-- 
1.7.4.1

Re: [PATCH 4/4] Fix handling asynchronous plugin reads

[Luiz Augusto von Dentz]

Hi,

On Tue, Jun 7, 2011 at 10:10 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
> 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
> stream, causing obex_object_set_io_flags() remove the watch only when the
> streaming is still running (i.e. not suspended).
> ---
>  src/obex-priv.h |    1 +
>  src/obex.c      |   15 ++++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/obex-priv.h b/src/obex-priv.h
> index 8c722dc..54ec991 100644
> --- a/src/obex-priv.h
> +++ b/src/obex-priv.h
> @@ -46,6 +46,7 @@ struct obex_session {
>        obex_object_t *obj;
>        struct obex_mime_type_driver *driver;
>        gboolean streaming;
> +       gboolean stream_running;
>  };

I guess we can call this 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 659f39b..5f437c7 100644
> --- a/src/obex.c
> +++ b/src/obex.c
> @@ -311,6 +311,7 @@ static void os_reset_session(struct obex_session *os)
>        os->offset = 0;
>        os->size = OBJECT_SIZE_DELETE;
>        os->streaming = FALSE;
> +       os->stream_running = FALSE;
>  }
>
>  static void obex_session_free(struct obex_session *os)
> @@ -670,6 +671,7 @@ static int obex_write_stream(struct obex_session *os,
>                        OBEX_ObjectAddHeader(obex, obj, hi, hd, 0,
>                                                OBEX_FL_STREAM_START);
>                        os->streaming = TRUE;
> +                       os->stream_running = TRUE;
>                }
>
>                hd.bs = os->buf;
> @@ -698,6 +700,8 @@ static void suspend_stream(struct obex_session *os, obex_t *obex,
>  {
>        obex_headerdata_t hd;
>
> +       os->stream_running = FALSE;
> +
>        if (os->streaming) {
>                OBEX_SuspendRequest(obex, obj);
>                return;
> @@ -731,10 +735,15 @@ proceed:
>        if (ret < 0) {
>                os_set_response(os->obj, err);
>                OBEX_CancelRequest(os->obex, TRUE);
> -       } else
> +       } else {
> +               os->stream_running = TRUE;
>                OBEX_ResumeRequest(os->obex);
> +       }
>
> -       return FALSE;
> +       if (os->stream_running)
> +               return FALSE;
> +       else
> +               return TRUE;
>  }
>
>  static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
> @@ -1225,7 +1234,7 @@ static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
>        case OBEX_EV_STREAMEMPTY:
>                err = obex_write_stream(os, obex, obj);
>                if (err == -EAGAIN) {
> -                       OBEX_SuspendRequest(obex, obj);
> +                       suspend_stream(os, obex, obj);
>                        os->obj = obj;
>                        os->driver->set_io_watch(os->object, handle_async_io,
>                                                                        os);
> --

I guess we should be calling suspend_stream whenever we suspend the
stream so we set suspended/stream_running, also don't we need a
resume_stream to reset the flag?



-- 
Luiz Augusto von Dentz
Computer Engineer

Various GET functionality fixes (obsoletes: Renewed new missed calls handling)

[Slawomir Bochenski]

Some updates.

Now the function for suspending is named suspend_get() as the suspending
mechanism introduced is only related to GET requests. And this is also
why it is not called in every place we do OBEX_SuspendRequest().

The variable is named suspended now, as you suggested.

I don't think there is any need for additional function for resuming
request.  The only place this is done is handle_async_io(). 

[PATCH 3/4] Modify obex_write_stream logic

[Slawomir Bochenski]

PBAP and MAP profiles present a requirement that in case of multi-packet
responses, application parameter header shall be send in the first
packet.

Starting body streaming in OpenOBEX adds body header immediately and
queues any other added header for sending after the streaming is
finished. In order to get desired behaviour streaming mode should be
started after sending all other headers.

This patch modifies obex_write_stream() to do this. 'stream' argument to
service driver get() function is removed, as it is no longer needed -
streaming is started automatically when getting first portion of body
data.

Also previously when the 'stream' argument was set to false, it was only
possible to add single header, as there was no loop nor appropriate
suspending involved (which is required in order to not finish
non-streaming request immediately).

Now suspending in non-streaming case is achieved by adding OBEX_HDR_EMPTY
with OBEX_FL_SUSPEND flag set, so OpenOBEX will send whatever data is
available and suspend after that.
---
 plugins/ftp.c           |    6 +--
 plugins/ftp.h           |    3 +-
 plugins/irmc.c          |    3 +-
 plugins/mas.c           |    5 +--
 plugins/opp.c           |    6 +--
 plugins/pbap.c          |    7 +---
 plugins/pcsuite.c       |    4 +-
 plugins/syncevolution.c |    5 +--
 src/obex-priv.h         |    1 +
 src/obex.c              |  100 ++++++++++++++++++++++++++++++-----------------
 src/service.h           |    2 +-
 11 files changed, 75 insertions(+), 67 deletions(-)

diff --git a/plugins/ftp.c b/plugins/ftp.c
index 79223bf..4962326 100644
--- a/plugins/ftp.c
+++ b/plugins/ftp.c
@@ -213,8 +213,7 @@ void *ftp_connect(struct obex_session *os, int *err)
 	return ftp;
 }
 
-int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
-							void *user_data)
+int ftp_get(struct obex_session *os, obex_object_t *obj, void *user_data)
 {
 	struct ftp_session *ftp = user_data;
 	const char *type = obex_get_type(os);
@@ -229,9 +228,6 @@ int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
 	if (ret < 0)
 		return ret;
 
-	if (stream)
-		*stream = TRUE;
-
 	return 0;
 }
 
diff --git a/plugins/ftp.h b/plugins/ftp.h
index 2374125..d929cde 100644
--- a/plugins/ftp.h
+++ b/plugins/ftp.h
@@ -22,8 +22,7 @@
  */
 
 void *ftp_connect(struct obex_session *os, int *err);
-int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
-							void *user_data);
+int ftp_get(struct obex_session *os, obex_object_t *obj, void *user_data);
 int ftp_chkput(struct obex_session *os, void *user_data);
 int ftp_put(struct obex_session *os, obex_object_t *obj, void *user_data);
 int ftp_setpath(struct obex_session *os, obex_object_t *obj, void *user_data);
diff --git a/plugins/irmc.c b/plugins/irmc.c
index cd7f386..cc0b9db 100644
--- a/plugins/irmc.c
+++ b/plugins/irmc.c
@@ -233,7 +233,7 @@ static void *irmc_connect(struct obex_session *os, int *err)
 }
 
 static int irmc_get(struct obex_session *os, obex_object_t *obj,
-		gboolean *stream, void *user_data)
+							void *user_data)
 {
 	struct irmc_session *irmc = user_data;
 	const char *type = obex_get_type(os);
@@ -244,7 +244,6 @@ static int irmc_get(struct obex_session *os, obex_object_t *obj,
 	DBG("name %s type %s irmc %p", name, type ? type : "NA", irmc);
 
 	path = g_strdup(name);
-	*stream = TRUE;
 
 	ret = obex_get_stream_start(os, path);
 
diff --git a/plugins/mas.c b/plugins/mas.c
index d13625c..08e47a2 100644
--- a/plugins/mas.c
+++ b/plugins/mas.c
@@ -134,8 +134,7 @@ static void mas_disconnect(struct obex_session *os, void *user_data)
 	mas_clean(mas);
 }
 
-static int mas_get(struct obex_session *os, obex_object_t *obj,
-					gboolean *stream, void *user_data)
+static int mas_get(struct obex_session *os, obex_object_t *obj, void *user_data)
 {
 	struct mas_session *mas = user_data;
 	const char *type = obex_get_type(os);
@@ -148,8 +147,6 @@ static int mas_get(struct obex_session *os, obex_object_t *obj,
 	if (type == NULL)
 		return -EBADR;
 
-	*stream = FALSE;
-
 	ret = obex_get_stream_start(os, name);
 	if (ret < 0)
 		goto failed;
diff --git a/plugins/opp.c b/plugins/opp.c
index 17c4356..4f0ed08 100644
--- a/plugins/opp.c
+++ b/plugins/opp.c
@@ -170,8 +170,7 @@ static int opp_put(struct obex_session *os, obex_object_t *obj,
 	return 0;
 }
 
-static int opp_get(struct obex_session *os, obex_object_t *obj,
-			gboolean *stream, void *user_data)
+static int opp_get(struct obex_session *os, obex_object_t *obj, void *user_data)
 {
 	const char *type;
 
@@ -190,9 +189,6 @@ static int opp_get(struct obex_session *os, obex_object_t *obj,
 	} else
 		return -EPERM;
 
-	if (stream)
-		*stream = TRUE;
-
 	return 0;
 }
 
diff --git a/plugins/pbap.c b/plugins/pbap.c
index 0356ae7..a5b0117 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -625,7 +625,7 @@ static void *pbap_connect(struct obex_session *os, int *err)
 }
 
 static int pbap_get(struct obex_session *os, obex_object_t *obj,
-					gboolean *stream, void *user_data)
+							void *user_data)
 {
 	struct pbap_session *pbap = user_data;
 	const char *type = obex_get_type(os);
@@ -662,8 +662,6 @@ static int pbap_get(struct obex_session *os, obex_object_t *obj,
 			path = g_strdup(name);
 		else
 			path = g_build_filename("/", name, NULL);
-
-		*stream = (params->maxlistcount == 0 ? FALSE : TRUE);
 	} else if (strcmp(type, VCARDLISTING_TYPE) == 0) {
 		/* Always relative */
 		if (!name || strlen(name) == 0)
@@ -672,12 +670,9 @@ static int pbap_get(struct obex_session *os, obex_object_t *obj,
 		else
 			/* Current folder + relative path */
 			path = g_build_filename(pbap->folder, name, NULL);
-
-		*stream = (params->maxlistcount == 0 ? FALSE : TRUE);
 	} else if (strcmp(type, VCARDENTRY_TYPE) == 0) {
 		/* File name only */
 		path = g_strdup(name);
-		*stream = TRUE;
 	} else
 		return -EBADR;
 
diff --git a/plugins/pcsuite.c b/plugins/pcsuite.c
index 9cf65c9..318e186 100644
--- a/plugins/pcsuite.c
+++ b/plugins/pcsuite.c
@@ -181,13 +181,13 @@ fail:
 }
 
 static int pcsuite_get(struct obex_session *os, obex_object_t *obj,
-					gboolean *stream, void *user_data)
+							void *user_data)
 {
 	struct pcsuite_session *pcsuite = user_data;
 
 	DBG("%p", pcsuite);
 
-	return ftp_get(os, obj, stream, pcsuite->ftp);
+	return ftp_get(os, obj, pcsuite->ftp);
 }
 
 static int pcsuite_chkput(struct obex_session *os, void *user_data)
diff --git a/plugins/syncevolution.c b/plugins/syncevolution.c
index ea3eb7a..77c1bd6 100644
--- a/plugins/syncevolution.c
+++ b/plugins/syncevolution.c
@@ -250,11 +250,8 @@ static int synce_put(struct obex_session *os, obex_object_t *obj,
 }
 
 static int synce_get(struct obex_session *os, obex_object_t *obj,
-					gboolean *stream, void *user_data)
+							void *user_data)
 {
-	if (stream)
-		*stream = TRUE;
-
 	return obex_get_stream_start(os, NULL);
 }
 
diff --git a/src/obex-priv.h b/src/obex-priv.h
index 0caa0dd..8c722dc 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -45,6 +45,7 @@ struct obex_session {
 	obex_t *obex;
 	obex_object_t *obj;
 	struct obex_mime_type_driver *driver;
+	gboolean streaming;
 };
 
 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 e6585ca..1bdb6be 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -310,6 +310,7 @@ static void os_reset_session(struct obex_session *os)
 	os->pending = 0;
 	os->offset = 0;
 	os->size = OBJECT_SIZE_DELETE;
+	os->streaming = FALSE;
 }
 
 static void obex_session_free(struct obex_session *os)
@@ -638,43 +639,78 @@ static int obex_write_stream(struct obex_session *os,
 	if (os->object == NULL)
 		return -EIO;
 
-	len = os->driver->read(os->object, os->buf, os->tx_mtu, &hi);
-	if (len < 0) {
-		error("read(): %s (%zd)", strerror(-len), -len);
-		if (len == -EAGAIN)
+	do {
+		len = os->driver->read(os->object, os->buf, os->tx_mtu, &hi);
+		if (len < 0) {
+			error("read(): %s (%zd)", strerror(-len), -len);
+			if (len == -EAGAIN)
+				return len;
+			else if (len == -ENOSTR)
+				return 0;
+
+			g_free(os->buf);
+			os->buf = NULL;
 			return len;
-		else if (len == -ENOSTR)
+		}
+
+		if (len == 0) {
+			if (os->streaming) {
+				hd.bs = NULL;
+				OBEX_ObjectAddHeader(obex, obj, hi, hd, 0,
+							OBEX_FL_STREAM_DATAEND);
+			}
+			g_free(os->buf);
+			os->buf = NULL;
 			return 0;
+		}
 
-		g_free(os->buf);
-		os->buf = NULL;
-		return len;
-	}
+		if (!os->streaming && hi == OBEX_HDR_BODY) {
+			DBG("Starting streaming");
+			hd.bs = NULL;
+			OBEX_ObjectAddHeader(obex, obj, hi, hd, 0,
+						OBEX_FL_STREAM_START);
+			os->streaming = TRUE;
+		}
 
-	hd.bs = os->buf;
+		hd.bs = os->buf;
 
-	switch (hi) {
-	case OBEX_HDR_BODY:
-		flags = len ? OBEX_FL_STREAM_DATA : OBEX_FL_STREAM_DATAEND;
-		break;
-	case OBEX_HDR_APPARAM:
-		flags =  0;
-		break;
-	default:
-		error("read(): unkown header type %u", hi);
-		return -EIO;
-	}
+		switch (hi) {
+		case OBEX_HDR_BODY:
+			flags = OBEX_FL_STREAM_DATA;
+			break;
+		case OBEX_HDR_APPARAM:
+			flags = 0;
+			break;
+		default:
+			error("read(): unkown header type %u", hi);
+			return -EIO;
+		}
 
-	OBEX_ObjectAddHeader(obex, obj, hi, hd, len, flags);
+		OBEX_ObjectAddHeader(obex, obj, hi, hd, len, flags);
 
-	if (len == 0) {
-		g_free(os->buf);
-		os->buf = NULL;
-	}
+	} while (!os->streaming);
 
 	return 0;
 }
 
+static void suspend_get(struct obex_session *os, obex_t *obex,
+						obex_object_t *obj)
+{
+	obex_headerdata_t hd;
+
+	if (os->streaming) {
+		OBEX_SuspendRequest(obex, obj);
+		return;
+	}
+
+	hd.bs = NULL;
+	OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_EMPTY, hd, 0, OBEX_FL_SUSPEND);
+	/* If there were no headers in the queue after the one with
+	 * the suspend flag set, OpenOBEX would finalize the request.
+	 */
+	OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_EMPTY, hd, 0, 0);
+}
+
 static gboolean handle_async_io(void *object, int flags, int err,
 						void *user_data)
 {
@@ -704,7 +740,6 @@ proceed:
 static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
 {
 	obex_headerdata_t hd;
-	gboolean stream;
 	unsigned int hlen;
 	uint8_t hi;
 	int err;
@@ -781,7 +816,7 @@ static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
 		return;
 	}
 
-	err = os->service->get(os, obj, &stream, os->service_data);
+	err = os->service->get(os, obj, os->service_data);
 
 	if (err < 0)
 		goto done;
@@ -800,16 +835,9 @@ static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
 		goto done;
 	}
 
-	if (stream)
-		/* Standard data stream */
-		OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_BODY, hd, 0,
-						OBEX_FL_STREAM_START);
-
-	/* Try to write to stream and suspend the stream immediately
-	 * if no data available to send. */
 	err = obex_write_stream(os, obex, obj);
 	if (err == -EAGAIN) {
-		OBEX_SuspendRequest(obex, obj);
+		suspend_get(os, obex, obj);
 		os->obj = obj;
 		os->driver->set_io_watch(os->object, handle_async_io, os);
 		return;
diff --git a/src/service.h b/src/service.h
index a844885..4d9d683 100644
--- a/src/service.h
+++ b/src/service.h
@@ -33,7 +33,7 @@ struct obex_service_driver {
 	void *(*connect) (struct obex_session *os, int *err);
 	void (*progress) (struct obex_session *os, void *user_data);
 	int (*get) (struct obex_session *os, obex_object_t *obj,
-			gboolean *stream, void *user_data);
+							void *user_data);
 	int (*put) (struct obex_session *os, obex_object_t *obj,
 			void *user_data);
 	int (*chkput) (struct obex_session *os, void *user_data);
-- 
1.7.4.1

[PATCH 4/4] 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
stream, causing obex_object_set_io_flags() remove the watch only when the
streaming is still running (i.e. not suspended).
---
 src/obex-priv.h |    1 +
 src/obex.c      |   15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/obex-priv.h b/src/obex-priv.h
index 8c722dc..164bc78 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -46,6 +46,7 @@ struct obex_session {
 	obex_object_t *obj;
 	struct obex_mime_type_driver *driver;
 	gboolean streaming;
+	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 1bdb6be..00e64c6 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -311,6 +311,7 @@ static void os_reset_session(struct obex_session *os)
 	os->offset = 0;
 	os->size = OBJECT_SIZE_DELETE;
 	os->streaming = FALSE;
+	os->suspended = FALSE;
 }
 
 static void obex_session_free(struct obex_session *os)
@@ -698,6 +699,8 @@ static void suspend_get(struct obex_session *os, obex_t *obex,
 {
 	obex_headerdata_t hd;
 
+	os->suspended = TRUE;
+
 	if (os->streaming) {
 		OBEX_SuspendRequest(obex, obj);
 		return;
@@ -728,13 +731,19 @@ 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 < 0) {
 		os_set_response(os->obj, err);
 		OBEX_CancelRequest(os->obex, TRUE);
-	} else
+	} else {
 		OBEX_ResumeRequest(os->obex);
+	}
 
-	return FALSE;
+	if (os->suspended)
+		return TRUE;
+	else
+		return FALSE;
 }
 
 static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
@@ -1225,7 +1234,7 @@ static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
 	case OBEX_EV_STREAMEMPTY:
 		err = obex_write_stream(os, obex, obj);
 		if (err == -EAGAIN) {
-			OBEX_SuspendRequest(obex, obj);
+			suspend_get(os, obex, obj);
 			os->obj = obj;
 			os->driver->set_io_watch(os->object, handle_async_io,
 									os);
-- 
1.7.4.1

[PATCH v3 4/4] 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      |   19 +++++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/obex-priv.h b/src/obex-priv.h
index 8c722dc..164bc78 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -46,6 +46,7 @@ struct obex_session {
 	obex_object_t *obj;
 	struct obex_mime_type_driver *driver;
 	gboolean streaming;
+	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 1bdb6be..99fedc3 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -311,6 +311,7 @@ static void os_reset_session(struct obex_session *os)
 	os->offset = 0;
 	os->size = OBJECT_SIZE_DELETE;
 	os->streaming = FALSE;
+	os->suspended = FALSE;
 }
 
 static void obex_session_free(struct obex_session *os)
@@ -698,6 +699,8 @@ static void suspend_get(struct obex_session *os, obex_t *obex,
 {
 	obex_headerdata_t hd;
 
+	os->suspended = TRUE;
+
 	if (os->streaming) {
 		OBEX_SuspendRequest(obex, obj);
 		return;
@@ -728,13 +731,21 @@ static gboolean handle_async_io(void *object, int flags, int err,
 		ret = obex_read_stream(os, os->obex, os->obj);
 
 proceed:
-	if (ret < 0) {
+	os->suspended = FALSE;
+
+	if (ret == -EAGAIN) {
+		suspend_get(os, os->obex, os->obj);
+	} else if (ret < 0) {
 		os_set_response(os->obj, err);
 		OBEX_CancelRequest(os->obex, TRUE);
-	} else
+	} else {
 		OBEX_ResumeRequest(os->obex);
+	}
 
-	return FALSE;
+	if (os->suspended)
+		return TRUE;
+	else
+		return FALSE;
 }
 
 static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
@@ -1225,7 +1236,7 @@ static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
 	case OBEX_EV_STREAMEMPTY:
 		err = obex_write_stream(os, obex, obj);
 		if (err == -EAGAIN) {
-			OBEX_SuspendRequest(obex, obj);
+			suspend_get(os, obex, obj);
 			os->obj = obj;
 			os->driver->set_io_watch(os->object, handle_async_io,
 									os);
-- 
1.7.4.1

Re: [PATCH v3 4/4] Fix handling asynchronous plugin reads

[Luiz Augusto von Dentz]

Hi,

On Mon, Jun 13, 2011 at 8:24 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
> 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      |   19 +++++++++++++++----
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/src/obex-priv.h b/src/obex-priv.h
> index 8c722dc..164bc78 100644
> --- a/src/obex-priv.h
> +++ b/src/obex-priv.h
> @@ -46,6 +46,7 @@ struct obex_session {
>        obex_object_t *obj;
>        struct obex_mime_type_driver *driver;
>        gboolean streaming;
> +       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 1bdb6be..99fedc3 100644
> --- a/src/obex.c
> +++ b/src/obex.c
> @@ -311,6 +311,7 @@ static void os_reset_session(struct obex_session *os)
>        os->offset = 0;
>        os->size = OBJECT_SIZE_DELETE;
>        os->streaming = FALSE;
> +       os->suspended = FALSE;
>  }
>
>  static void obex_session_free(struct obex_session *os)
> @@ -698,6 +699,8 @@ static void suspend_get(struct obex_session *os, obex_t *obex,
>  {
>        obex_headerdata_t hd;
>
> +       os->suspended = TRUE;
> +
>        if (os->streaming) {
>                OBEX_SuspendRequest(obex, obj);
>                return;
> @@ -728,13 +731,21 @@ static gboolean handle_async_io(void *object, int flags, int err,
>                ret = obex_read_stream(os, os->obex, os->obj);
>
>  proceed:
> -       if (ret < 0) {
> +       os->suspended = FALSE;
> +
> +       if (ret == -EAGAIN) {
> +               suspend_get(os, os->obex, os->obj);
> +       } else if (ret < 0) {
>                os_set_response(os->obj, err);
>                OBEX_CancelRequest(os->obex, TRUE);
> -       } else
> +       } else {
>                OBEX_ResumeRequest(os->obex);
> +       }

One line statements do not need {

> -       return FALSE;
> +       if (os->suspended)
> +               return TRUE;
> +       else
> +               return FALSE;
>  }

I guess here you can just return os->suspended.

>  static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
> @@ -1225,7 +1236,7 @@ static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
>        case OBEX_EV_STREAMEMPTY:
>                err = obex_write_stream(os, obex, obj);
>                if (err == -EAGAIN) {
> -                       OBEX_SuspendRequest(obex, obj);
> +                       suspend_get(os, obex, obj);
>                        os->obj = obj;
>                        os->driver->set_io_watch(os->object, handle_async_io,
>                                                                        os);
> --
> 1.7.4.1
>
> --
> 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
>



-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH v3 4/4] Fix handling asynchronous plugin reads

[Slawomir Bochenski]

On Tue, Jun 14, 2011 at 8:33 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>> +       if (ret == -EAGAIN) {
>> +               suspend_get(os, os->obex, os->obj);
>> +       } else if (ret < 0) {
>>                os_set_response(os->obj, err);
>>                OBEX_CancelRequest(os->obex, TRUE);
>> -       } else
>> +       } else {
>>                OBEX_ResumeRequest(os->obex);
>> +       }
>
> One line statements do not need {
Kernel coding style states that this is not true when another branch
in the if-else has more than one statement. Then braces should be used
for all branches.
>
>> -       return FALSE;
>> +       if (os->suspended)
>> +               return TRUE;
>> +       else
>> +               return FALSE;
>>  }
>
> I guess here you can just return os->suspended.
I guess I can.

-- 
Slawomir Bochenski

Re: [PATCH v3 4/4] Fix handling asynchronous plugin reads

[Johan Hedberg]

Hi Slawek,

On Tue, Jun 14, 2011, Slawomir Bochenski wrote:
> On Tue, Jun 14, 2011 at 8:33 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >> +       if (ret == -EAGAIN) {
> >> +               suspend_get(os, os->obex, os->obj);
> >> +       } else if (ret < 0) {
> >>                os_set_response(os->obj, err);
> >>                OBEX_CancelRequest(os->obex, TRUE);
> >> -       } else
> >> +       } else {
> >>                OBEX_ResumeRequest(os->obex);
> >> +       }
> >
> > One line statements do not need {
> Kernel coding style states that this is not true when another branch
> in the if-else has more than one statement. Then braces should be used
> for all branches.
> >
> >> -       return FALSE;
> >> +       if (os->suspended)
> >> +               return TRUE;
> >> +       else
> >> +               return FALSE;
> >>  }
> >
> > I guess here you can just return os->suspended.
> I guess I can.

I've applied 1/4 and 2/4 since they seem fine. Luiz had some more
comments on 3/4 (I assume he'll send an email soon) and I still didn't
see an update on 4/4 to address this coding style issue Luiz pointed
out.

Johan

Re: [PATCH 3/4] Modify obex_write_stream logic

[Luiz Augusto von Dentz]

Hi,

On Mon, Jun 13, 2011 at 12:34 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
> PBAP and MAP profiles present a requirement that in case of multi-packet
> responses, application parameter header shall be send in the first
> packet.
>
> Starting body streaming in OpenOBEX adds body header immediately and
> queues any other added header for sending after the streaming is
> finished. In order to get desired behaviour streaming mode should be
> started after sending all other headers.
>
> This patch modifies obex_write_stream() to do this. 'stream' argument to
> service driver get() function is removed, as it is no longer needed -
> streaming is started automatically when getting first portion of body
> data.
>
> Also previously when the 'stream' argument was set to false, it was only
> possible to add single header, as there was no loop nor appropriate
> suspending involved (which is required in order to not finish
> non-streaming request immediately).
>
> Now suspending in non-streaming case is achieved by adding OBEX_HDR_EMPTY
> with OBEX_FL_SUSPEND flag set, so OpenOBEX will send whatever data is
> available and suspend after that.
> ---
>  plugins/ftp.c           |    6 +--
>  plugins/ftp.h           |    3 +-
>  plugins/irmc.c          |    3 +-
>  plugins/mas.c           |    5 +--
>  plugins/opp.c           |    6 +--
>  plugins/pbap.c          |    7 +---
>  plugins/pcsuite.c       |    4 +-
>  plugins/syncevolution.c |    5 +--
>  src/obex-priv.h         |    1 +
>  src/obex.c              |  100 ++++++++++++++++++++++++++++++-----------------
>  src/service.h           |    2 +-
>  11 files changed, 75 insertions(+), 67 deletions(-)
>
> diff --git a/plugins/ftp.c b/plugins/ftp.c
> index 79223bf..4962326 100644
> --- a/plugins/ftp.c
> +++ b/plugins/ftp.c
> @@ -213,8 +213,7 @@ void *ftp_connect(struct obex_session *os, int *err)
>        return ftp;
>  }
>
> -int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
> -                                                       void *user_data)
> +int ftp_get(struct obex_session *os, obex_object_t *obj, void *user_data)
>  {
>        struct ftp_session *ftp = user_data;
>        const char *type = obex_get_type(os);
> @@ -229,9 +228,6 @@ int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
>        if (ret < 0)
>                return ret;
>
> -       if (stream)
> -               *stream = TRUE;
> -
>        return 0;
>  }
>
> diff --git a/plugins/ftp.h b/plugins/ftp.h
> index 2374125..d929cde 100644
> --- a/plugins/ftp.h
> +++ b/plugins/ftp.h
> @@ -22,8 +22,7 @@
>  */
>
>  void *ftp_connect(struct obex_session *os, int *err);
> -int ftp_get(struct obex_session *os, obex_object_t *obj, gboolean *stream,
> -                                                       void *user_data);
> +int ftp_get(struct obex_session *os, obex_object_t *obj, void *user_data);
>  int ftp_chkput(struct obex_session *os, void *user_data);
>  int ftp_put(struct obex_session *os, obex_object_t *obj, void *user_data);
>  int ftp_setpath(struct obex_session *os, obex_object_t *obj, void *user_data);
> diff --git a/plugins/irmc.c b/plugins/irmc.c
> index cd7f386..cc0b9db 100644
> --- a/plugins/irmc.c
> +++ b/plugins/irmc.c
> @@ -233,7 +233,7 @@ static void *irmc_connect(struct obex_session *os, int *err)
>  }
>
>  static int irmc_get(struct obex_session *os, obex_object_t *obj,
> -               gboolean *stream, void *user_data)
> +                                                       void *user_data)
>  {
>        struct irmc_session *irmc = user_data;
>        const char *type = obex_get_type(os);
> @@ -244,7 +244,6 @@ static int irmc_get(struct obex_session *os, obex_object_t *obj,
>        DBG("name %s type %s irmc %p", name, type ? type : "NA", irmc);
>
>        path = g_strdup(name);
> -       *stream = TRUE;
>
>        ret = obex_get_stream_start(os, path);
>
> diff --git a/plugins/mas.c b/plugins/mas.c
> index d13625c..08e47a2 100644
> --- a/plugins/mas.c
> +++ b/plugins/mas.c
> @@ -134,8 +134,7 @@ static void mas_disconnect(struct obex_session *os, void *user_data)
>        mas_clean(mas);
>  }
>
> -static int mas_get(struct obex_session *os, obex_object_t *obj,
> -                                       gboolean *stream, void *user_data)
> +static int mas_get(struct obex_session *os, obex_object_t *obj, void *user_data)
>  {
>        struct mas_session *mas = user_data;
>        const char *type = obex_get_type(os);
> @@ -148,8 +147,6 @@ static int mas_get(struct obex_session *os, obex_object_t *obj,
>        if (type == NULL)
>                return -EBADR;
>
> -       *stream = FALSE;
> -
>        ret = obex_get_stream_start(os, name);
>        if (ret < 0)
>                goto failed;
> diff --git a/plugins/opp.c b/plugins/opp.c
> index 17c4356..4f0ed08 100644
> --- a/plugins/opp.c
> +++ b/plugins/opp.c
> @@ -170,8 +170,7 @@ static int opp_put(struct obex_session *os, obex_object_t *obj,
>        return 0;
>  }
>
> -static int opp_get(struct obex_session *os, obex_object_t *obj,
> -                       gboolean *stream, void *user_data)
> +static int opp_get(struct obex_session *os, obex_object_t *obj, void *user_data)
>  {
>        const char *type;
>
> @@ -190,9 +189,6 @@ static int opp_get(struct obex_session *os, obex_object_t *obj,
>        } else
>                return -EPERM;
>
> -       if (stream)
> -               *stream = TRUE;
> -
>        return 0;
>  }
>
> diff --git a/plugins/pbap.c b/plugins/pbap.c
> index 0356ae7..a5b0117 100644
> --- a/plugins/pbap.c
> +++ b/plugins/pbap.c
> @@ -625,7 +625,7 @@ static void *pbap_connect(struct obex_session *os, int *err)
>  }
>
>  static int pbap_get(struct obex_session *os, obex_object_t *obj,
> -                                       gboolean *stream, void *user_data)
> +                                                       void *user_data)
>  {
>        struct pbap_session *pbap = user_data;
>        const char *type = obex_get_type(os);
> @@ -662,8 +662,6 @@ static int pbap_get(struct obex_session *os, obex_object_t *obj,
>                        path = g_strdup(name);
>                else
>                        path = g_build_filename("/", name, NULL);
> -
> -               *stream = (params->maxlistcount == 0 ? FALSE : TRUE);
>        } else if (strcmp(type, VCARDLISTING_TYPE) == 0) {
>                /* Always relative */
>                if (!name || strlen(name) == 0)
> @@ -672,12 +670,9 @@ static int pbap_get(struct obex_session *os, obex_object_t *obj,
>                else
>                        /* Current folder + relative path */
>                        path = g_build_filename(pbap->folder, name, NULL);
> -
> -               *stream = (params->maxlistcount == 0 ? FALSE : TRUE);
>        } else if (strcmp(type, VCARDENTRY_TYPE) == 0) {
>                /* File name only */
>                path = g_strdup(name);
> -               *stream = TRUE;
>        } else
>                return -EBADR;
>
> diff --git a/plugins/pcsuite.c b/plugins/pcsuite.c
> index 9cf65c9..318e186 100644
> --- a/plugins/pcsuite.c
> +++ b/plugins/pcsuite.c
> @@ -181,13 +181,13 @@ fail:
>  }
>
>  static int pcsuite_get(struct obex_session *os, obex_object_t *obj,
> -                                       gboolean *stream, void *user_data)
> +                                                       void *user_data)
>  {
>        struct pcsuite_session *pcsuite = user_data;
>
>        DBG("%p", pcsuite);
>
> -       return ftp_get(os, obj, stream, pcsuite->ftp);
> +       return ftp_get(os, obj, pcsuite->ftp);
>  }
>
>  static int pcsuite_chkput(struct obex_session *os, void *user_data)
> diff --git a/plugins/syncevolution.c b/plugins/syncevolution.c
> index ea3eb7a..77c1bd6 100644
> --- a/plugins/syncevolution.c
> +++ b/plugins/syncevolution.c
> @@ -250,11 +250,8 @@ static int synce_put(struct obex_session *os, obex_object_t *obj,
>  }
>
>  static int synce_get(struct obex_session *os, obex_object_t *obj,
> -                                       gboolean *stream, void *user_data)
> +                                                       void *user_data)
>  {
> -       if (stream)
> -               *stream = TRUE;
> -
>        return obex_get_stream_start(os, NULL);
>  }
>
> diff --git a/src/obex-priv.h b/src/obex-priv.h
> index 0caa0dd..8c722dc 100644
> --- a/src/obex-priv.h
> +++ b/src/obex-priv.h
> @@ -45,6 +45,7 @@ struct obex_session {
>        obex_t *obex;
>        obex_object_t *obj;
>        struct obex_mime_type_driver *driver;
> +       gboolean streaming;
>  };
>
>  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 e6585ca..1bdb6be 100644
> --- a/src/obex.c
> +++ b/src/obex.c
> @@ -310,6 +310,7 @@ static void os_reset_session(struct obex_session *os)
>        os->pending = 0;
>        os->offset = 0;
>        os->size = OBJECT_SIZE_DELETE;
> +       os->streaming = FALSE;
>  }
>
>  static void obex_session_free(struct obex_session *os)
> @@ -638,43 +639,78 @@ static int obex_write_stream(struct obex_session *os,
>        if (os->object == NULL)
>                return -EIO;
>
> -       len = os->driver->read(os->object, os->buf, os->tx_mtu, &hi);
> -       if (len < 0) {
> -               error("read(): %s (%zd)", strerror(-len), -len);
> -               if (len == -EAGAIN)
> +       do {
> +               len = os->driver->read(os->object, os->buf, os->tx_mtu, &hi);
> +               if (len < 0) {
> +                       error("read(): %s (%zd)", strerror(-len), -len);
> +                       if (len == -EAGAIN)
> +                               return len;
> +                       else if (len == -ENOSTR)
> +                               return 0;
> +
> +                       g_free(os->buf);
> +                       os->buf = NULL;
>                        return len;
> -               else if (len == -ENOSTR)
> +               }
> +
> +               if (len == 0) {
> +                       if (os->streaming) {
> +                               hd.bs = NULL;
> +                               OBEX_ObjectAddHeader(obex, obj, hi, hd, 0,
> +                                                       OBEX_FL_STREAM_DATAEND);
> +                       }
> +                       g_free(os->buf);
> +                       os->buf = NULL;
>                        return 0;
> +               }


So this while loop is a bit dangerous since it is supposing to be
doing io, so what about separating the read of apparam and body in two
callbacks, the apparam could probably be consider a metadata of you
are actually accessing so separating those might be a good idea in the
long term. If so then read should probably only read the data itself
and never mix with apparams, then we have a e.g. read_apparams which
is only called once before reading the actual data, how about that?

> -               g_free(os->buf);
> -               os->buf = NULL;
> -               return len;
> -       }
> +               if (!os->streaming && hi == OBEX_HDR_BODY) {
> +                       DBG("Starting streaming");
> +                       hd.bs = NULL;
> +                       OBEX_ObjectAddHeader(obex, obj, hi, hd, 0,
> +                                               OBEX_FL_STREAM_START);
> +                       os->streaming = TRUE;
> +               }
>
> -       hd.bs = os->buf;
> +               hd.bs = os->buf;
>
> -       switch (hi) {
> -       case OBEX_HDR_BODY:
> -               flags = len ? OBEX_FL_STREAM_DATA : OBEX_FL_STREAM_DATAEND;
> -               break;
> -       case OBEX_HDR_APPARAM:
> -               flags =  0;
> -               break;
> -       default:
> -               error("read(): unkown header type %u", hi);
> -               return -EIO;
> -       }
> +               switch (hi) {
> +               case OBEX_HDR_BODY:
> +                       flags = OBEX_FL_STREAM_DATA;
> +                       break;
> +               case OBEX_HDR_APPARAM:
> +                       flags = 0;
> +                       break;
> +               default:
> +                       error("read(): unkown header type %u", hi);
> +                       return -EIO;
> +               }
>
> -       OBEX_ObjectAddHeader(obex, obj, hi, hd, len, flags);
> +               OBEX_ObjectAddHeader(obex, obj, hi, hd, len, flags);
>
> -       if (len == 0) {
> -               g_free(os->buf);
> -               os->buf = NULL;
> -       }
> +       } while (!os->streaming);
>
>        return 0;
>  }
>
> +static void suspend_get(struct obex_session *os, obex_t *obex,
> +                                               obex_object_t *obj)
> +{
> +       obex_headerdata_t hd;
> +
> +       if (os->streaming) {
> +               OBEX_SuspendRequest(obex, obj);
> +               return;
> +       }
> +
> +       hd.bs = NULL;
> +       OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_EMPTY, hd, 0, OBEX_FL_SUSPEND);
> +       /* If there were no headers in the queue after the one with
> +        * the suspend flag set, OpenOBEX would finalize the request.
> +        */
> +       OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_EMPTY, hd, 0, 0);
> +}
> +
>  static gboolean handle_async_io(void *object, int flags, int err,
>                                                void *user_data)
>  {
> @@ -704,7 +740,6 @@ proceed:
>  static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
>  {
>        obex_headerdata_t hd;
> -       gboolean stream;
>        unsigned int hlen;
>        uint8_t hi;
>        int err;
> @@ -781,7 +816,7 @@ static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
>                return;
>        }
>
> -       err = os->service->get(os, obj, &stream, os->service_data);
> +       err = os->service->get(os, obj, os->service_data);
>
>        if (err < 0)
>                goto done;
> @@ -800,16 +835,9 @@ static void cmd_get(struct obex_session *os, obex_t *obex, obex_object_t *obj)
>                goto done;
>        }
>
> -       if (stream)
> -               /* Standard data stream */
> -               OBEX_ObjectAddHeader(obex, obj, OBEX_HDR_BODY, hd, 0,
> -                                               OBEX_FL_STREAM_START);
> -
> -       /* Try to write to stream and suspend the stream immediately
> -        * if no data available to send. */
>        err = obex_write_stream(os, obex, obj);
>        if (err == -EAGAIN) {
> -               OBEX_SuspendRequest(obex, obj);
> +               suspend_get(os, obex, obj);
>                os->obj = obj;
>                os->driver->set_io_watch(os->object, handle_async_io, os);
>                return;
> diff --git a/src/service.h b/src/service.h
> index a844885..4d9d683 100644
> --- a/src/service.h
> +++ b/src/service.h
> @@ -33,7 +33,7 @@ struct obex_service_driver {
>        void *(*connect) (struct obex_session *os, int *err);
>        void (*progress) (struct obex_session *os, void *user_data);
>        int (*get) (struct obex_session *os, obex_object_t *obj,
> -                       gboolean *stream, void *user_data);
> +                                                       void *user_data);
>        int (*put) (struct obex_session *os, obex_object_t *obj,
>                        void *user_data);
>        int (*chkput) (struct obex_session *os, void *user_data);
> --
> 1.7.4.1
>
>



-- 
Luiz Augusto von Dentz

Re: [PATCH 3/4] Modify obex_write_stream logic

[Johan Hedberg]

Hi Luiz,

On Wed, Jun 15, 2011, Luiz Augusto von Dentz wrote:
> So this while loop is a bit dangerous since it is supposing to be
> doing io, so what about separating the read of apparam and body in two
> callbacks, the apparam could probably be consider a metadata of you
> are actually accessing so separating those might be a good idea in the
> long term. If so then read should probably only read the data itself
> and never mix with apparams, then we have a e.g. read_apparams which
> is only called once before reading the actual data, how about that?

Seems more or less ok with me, but I'd call it get_apparams since it's
not really a stream of data like the body. FWIW, I've completely missed
the extension of the read callback to return other types of data besides
that which is contained within body headers. The original intent of it
was to only operate on the actual object body and it'd be nice if we
could get it back to that state again.

Johan

Re: [PATCH 3/4] Modify obex_write_stream logic

[Slawomir Bochenski]

On Wed, Jun 15, 2011 at 3:39 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Luiz,
>
> On Wed, Jun 15, 2011, Luiz Augusto von Dentz wrote:
>> So this while loop is a bit dangerous since it is supposing to be
>> doing io, so what about separating the read of apparam and body in two
>> callbacks, the apparam could probably be consider a metadata of you
>> are actually accessing so separating those might be a good idea in the
>> long term. If so then read should probably only read the data itself
>> and never mix with apparams, then we have a e.g. read_apparams which
>> is only called once before reading the actual data, how about that?

The loop is only used before we start streaming. That is because if
there was no streaming started, request will be finished after
returning from OpenOBEX callback. To still allow plugins to use the
original approach, the loop guarantees that there will be another
read() if the service plugin did not explicitly returned -EAGAIN, so
read() still decides explicitly when it wants the suspending to
happen, therefore it is perfectly aware of when to call
obex_object_set_io_flags().

This is just how OpenOBEX works - unless we are streaming body
headers, we should add headers when getting event and suspend if we
want to postpone finishing the request. I would not call this loop
"dangerous".

> Seems more or less ok with me, but I'd call it get_apparams since it's
> not really a stream of data like the body. FWIW, I've completely missed
> the extension of the read callback to return other types of data besides
> that which is contained within body headers. The original intent of it
> was to only operate on the actual object body and it'd be nice if we
> could get it back to that state again.

Well, we have also another possible header types and generally in some
near future a profile may need to use more than just application
parameters header and/or body headers (e.g. user defined headers).
This might also allow sending Length header asynchronously (which by
quick looking at code is not possible now).

With approach I presented the cost of changes is very low and
additionally makes it extremely easy to return all possible headers.
Also everything just works, together with support for asynchronous
I/O.

Adding additional callback will unnecessarily complicate the code.

-- 
Slawomir Bochenski

Re: [PATCH v3 4/4] Fix handling asynchronous plugin reads

[Slawomir Bochenski]

> I've applied 1/4 and 2/4 since they seem fine. Luiz had some more
> comments on 3/4 (I assume he'll send an email soon) and I still didn't
> see an update on 4/4 to address this coding style issue Luiz pointed
> out.

This 4/4 will be on hold until we settle with 3/4.

-- 
Slawomir Bochenski

Re: [PATCH 3/4] Modify obex_write_stream logic

[Luiz Augusto von Dentz]

Hi,

On Wed, Jun 15, 2011 at 5:59 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
>
> The loop is only used before we start streaming. That is because if
> there was no streaming started, request will be finished after
> returning from OpenOBEX callback. To still allow plugins to use the
> original approach, the loop guarantees that there will be another
> read() if the service plugin did not explicitly returned -EAGAIN, so
> read() still decides explicitly when it wants the suspending to
> happen, therefore it is perfectly aware of when to call
> obex_object_set_io_flags().
>
> This is just how OpenOBEX works - unless we are streaming body
> headers, we should add headers when getting event and suspend if we
> want to postpone finishing the request. I would not call this loop
> "dangerous".

I would say this is not a limitation of OpenOBEX, just the way we
implement async io in obexd.

> Well, we have also another possible header types and generally in some
> near future a profile may need to use more than just application
> parameters header and/or body headers (e.g. user defined headers).
> This might also allow sending Length header asynchronously (which by
> quick looking at code is not possible now).
>
> With approach I presented the cost of changes is very low and
> additionally makes it extremely easy to return all possible headers.
> Also everything just works, together with support for asynchronous
> I/O.
>
> Adding additional callback will unnecessarily complicate the code.

Sorry I have to disagree here, adding another callback probably
simplify the code a bit since we can have something like this:

if (!streaming && driver->get_apparams)
    driver->get_apparams...

So just drivers that support get_apparams will be called and that
should be enough to guarantee the apparams are added in the beginning
packet.

Custom headers are a different matter, right now we don't have any
plugin/driver who need them.

-- 
Luiz Augusto von Dentz

Re: [PATCH 3/4] Modify obex_write_stream logic

[Slawomir Bochenski]

On Thu, Jun 16, 2011 at 10:27 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Wed, Jun 15, 2011 at 5:59 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
>>
>> The loop is only used before we start streaming. That is because if
>> there was no streaming started, request will be finished after
>> returning from OpenOBEX callback. To still allow plugins to use the
>> original approach, the loop guarantees that there will be another
>> read() if the service plugin did not explicitly returned -EAGAIN, so
>> read() still decides explicitly when it wants the suspending to
>> happen, therefore it is perfectly aware of when to call
>> obex_object_set_io_flags().
>>
>> This is just how OpenOBEX works - unless we are streaming body
>> headers, we should add headers when getting event and suspend if we
>> want to postpone finishing the request. I would not call this loop
>> "dangerous".
>
> I would say this is not a limitation of OpenOBEX, just the way we
> implement async io in obexd.

We base asynchronous I/O on the fact the we are receiving
OBEX_EV_STREAMEMPTY. We get this only when we added body header with
OBEX_FL_STREAM_START. And this is "limitation" (I would not choose
this particular word) of OpenOBEX. Once again - if we return from a
callback, that means we finished replying to request, unless we
suspend it first (or like I've done it here, to send immediately what
is available to send - add appropriate header to make OpenOBEX suspend
it for us later).

>> Well, we have also another possible header types and generally in some
>> near future a profile may need to use more than just application
>> parameters header and/or body headers (e.g. user defined headers).
>> This might also allow sending Length header asynchronously (which by
>> quick looking at code is not possible now).
>>
>> With approach I presented the cost of changes is very low and
>> additionally makes it extremely easy to return all possible headers.
>> Also everything just works, together with support for asynchronous
>> I/O.
>>
>> Adding additional callback will unnecessarily complicate the code.
>
> Sorry I have to disagree here, adding another callback probably
> simplify the code a bit since we can have something like this:
>
> if (!streaming && driver->get_apparams)
>    driver->get_apparams...
>
> So just drivers that support get_apparams will be called and that
> should be enough to guarantee the apparams are added in the beginning
> packet.
>
> Custom headers are a different matter, right now we don't have any
> plugin/driver who need them.

So to make it work this would essentially require putting this into
obex_write_stream() (possibly doubling the code there), because we
would need to do the same checks (errors, -EAGAIN) and also it has to
be somehow supported by handle_async_io(). And yet removing
flexibility the approach presented gives, just because "right *now* no
plugin needs them".

Response is made up of headers of different kinds. This is no longer
only ftp and we are beyond following system read(). Thus we can really
use read() to read any kinds of response headers plugin wishes to
send. What's *really* wrong with that?

-- 
Slawomir Bochenski

Re: [PATCH 3/4] Modify obex_write_stream logic

[Slawomir Bochenski]

Another possibility I see to keep the flexibility and make read() be
used only for Body (as we rather should not add any headers after End
of Body) is to add ->get_header() which would be a general callback to
add any non-body headers plugin wishes to use.

-- 
Slawomir Bochenski

Re: [PATCH 3/4] Modify obex_write_stream logic

[Vinicius Costa Gomes]

Hi Luiz,

On 11:27 Thu 16 Jun, Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Wed, Jun 15, 2011 at 5:59 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
> >
> > The loop is only used before we start streaming. That is because if
> > there was no streaming started, request will be finished after
> > returning from OpenOBEX callback. To still allow plugins to use the
> > original approach, the loop guarantees that there will be another
> > read() if the service plugin did not explicitly returned -EAGAIN, so
> > read() still decides explicitly when it wants the suspending to
> > happen, therefore it is perfectly aware of when to call
> > obex_object_set_io_flags().
> >
> > This is just how OpenOBEX works - unless we are streaming body
> > headers, we should add headers when getting event and suspend if we
> > want to postpone finishing the request. I would not call this loop
> > "dangerous".
> 
> I would say this is not a limitation of OpenOBEX, just the way we
> implement async io in obexd.
> 
> > Well, we have also another possible header types and generally in some
> > near future a profile may need to use more than just application
> > parameters header and/or body headers (e.g. user defined headers).
> > This might also allow sending Length header asynchronously (which by
> > quick looking at code is not possible now).
> >
> > With approach I presented the cost of changes is very low and
> > additionally makes it extremely easy to return all possible headers.
> > Also everything just works, together with support for asynchronous
> > I/O.
> >
> > Adding additional callback will unnecessarily complicate the code.
> 
> Sorry I have to disagree here, adding another callback probably
> simplify the code a bit since we can have something like this:
> 
> if (!streaming && driver->get_apparams)
>     driver->get_apparams...
> 
> So just drivers that support get_apparams will be called and that
> should be enough to guarantee the apparams are added in the beginning
> packet.
> 
> Custom headers are a different matter, right now we don't have any
> plugin/driver who need them.

But we will have soon, BIP needs them. So we better start considering
how should we export it to the plugins.

> 
> -- 
> Luiz Augusto von Dentz
> --
> 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


Cheers,
-- 
Vinicius

Re: [PATCH 3/4] Modify obex_write_stream logic

[Luiz Augusto von Dentz]

Hi Vinicius,

On Thu, Jun 16, 2011 at 7:51 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
>> Sorry I have to disagree here, adding another callback probably
>> simplify the code a bit since we can have something like this:
>>
>> if (!streaming && driver->get_apparams)
>>     driver->get_apparams...
>>
>> So just drivers that support get_apparams will be called and that
>> should be enough to guarantee the apparams are added in the beginning
>> packet.
>>
>> Custom headers are a different matter, right now we don't have any
>> plugin/driver who need them.
>
> But we will have soon, BIP needs them. So we better start considering
> how should we export it to the plugins.

Sure, still a different plugin, maybe when BIP is to be introduced we
should actually give the OpenOBEX object to the drivers, so that each
plugin can add their own headers in whatever order they want, or have
an abstraction of the object e.g. obexd_object which the plugins can
use to add their own headers. How about that?

-- 
Luiz Augusto von Dentz

Re: [PATCH 3/4] Modify obex_write_stream logic

[Slawomir Bochenski]

On Fri, Jun 17, 2011 at 9:35 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>> But we will have soon, BIP needs them. So we better start considering
>> how should we export it to the plugins.
>
> Sure, still a different plugin, maybe when BIP is to be introduced we
> should actually give the OpenOBEX object to the drivers, so that each
> plugin can add their own headers in whatever order they want, or have
> an abstraction of the object e.g. obexd_object which the plugins can
> use to add their own headers. How about that?

The approach of direct accessing obex_object_t from within plugin was
used before (among other bad practices) and dropped more than year
ago.

-- 
Slawomir Bochenski