[PATCH obexd 1/4] Reverse order of calls in os_reset_session

[PATCH obexd 1/4] Reverse order of calls in os_reset_session

[Slawomir Bochenski]

This replaces:
service->get/put, mime->open, ..., service->reset, mime->close
logic, with a more appropriate:
service->get/put, mime->open, ..., mime->close, service->reset
---
 src/obex.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/obex.c b/src/obex.c
index d8f4648..938937c 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -284,8 +284,6 @@ static void os_session_mark_aborted(struct obex_session *os)
 static void os_reset_session(struct obex_session *os)
 {
 	os_session_mark_aborted(os);
-	if (os->service && os->service->reset)
-		os->service->reset(os, os->service_data);
 
 	if (os->object) {
 		os->driver->set_io_watch(os->object, NULL, NULL);
@@ -297,6 +295,9 @@ static void os_reset_session(struct obex_session *os)
 			os->driver->remove(os->path);
 	}
 
+	if (os->service && os->service->reset)
+		os->service->reset(os, os->service_data);
+
 	if (os->name) {
 		g_free(os->name);
 		os->name = NULL;
-- 
1.7.4.1

[PATCH obexd 2/4] Fix missing format in call to g_error_new

[Slawomir Bochenski]

---
 client/transfer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/client/transfer.c b/client/transfer.c
index 85139ac..b6994d1 100644
--- a/client/transfer.c
+++ b/client/transfer.c
@@ -143,7 +143,7 @@ static void obc_transfer_abort(struct obc_transfer *transfer)
 	if (callback) {
 		GError *err;
 
-		err = g_error_new(OBEX_IO_ERROR, -ECANCELED,
+		err = g_error_new(OBEX_IO_ERROR, -ECANCELED, "%s",
 							strerror(ECANCELED));
 		callback->func(transfer, transfer->transferred, err,
 							callback->data);
-- 
1.7.4.1

[PATCH obexd 3/4] Write pending bytes only if mime object is open

[Slawomir Bochenski]

Without this in case of "non-checked" PUT there were calls to write()
prior to any obex_put_stream_start().
---
 src/obex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/obex.c b/src/obex.c
index 938937c..38c5ca6 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -580,7 +580,7 @@ static int obex_read_stream(struct obex_session *os, obex_t *obex,
 		os->size = OBJECT_SIZE_UNKNOWN;
 
 	/* If there's something to write and we are able to write it */
-	if (os->pending > 0 && os->driver)
+	if (os->pending > 0 && os->driver && os->object)
 		goto write;
 
 	size = OBEX_ObjectReadStream(obex, obj, &buffer);
-- 
1.7.4.1

[PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Slawomir Bochenski]

OBEX_EV_REQ is the last thing that we are going to receive on PUT and
this is always going to be delivered. As the service driver will start
receiving data at or before its .put(), we can simply call flush() after
that. This also makes flush() usable in PUTs without length header.
---
 src/obex.c |   28 ++++++++++------------------
 1 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/src/obex.c b/src/obex.c
index 38c5ca6..2cabadb 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -625,11 +625,6 @@ write:
 		os->pending -= w;
 	}
 
-	/* Flush on EOS */
-	if (os->size != OBJECT_SIZE_UNKNOWN && os->size == os->offset &&
-							os->driver->flush)
-		return os->driver->flush(os->object) > 0 ? -EAGAIN : 0;
-
 	return 0;
 }
 
@@ -1140,19 +1135,16 @@ static void cmd_put(struct obex_session *os, obex_t *obex, obex_object_t *obj)
 		return;
 	}
 
-	/* Check if there is a body and it is not empty (size > 0), otherwise
-	   openobex won't notify us with OBEX_EV_STREAMAVAIL and it gonna reply
-	   right away */
-	if (os->size != 0)
-		return;
-
-	/* Flush immediatly since there is nothing to write so the driver
-	   has a chance to do something before we reply */
-	if (os->object && os->driver && os->driver->flush &&
-					os->driver->flush(os->object) > 0) {
-		OBEX_SuspendRequest(obex, obj);
-		os->obj = obj;
-		os->driver->set_io_watch(os->object, handle_async_io, os);
+	if (os->object && os->driver && os->driver->flush) {
+		err = os->driver->flush(os->object);
+		if (err == -EAGAIN) {
+			OBEX_SuspendRequest(obex, obj);
+			os->obj = obj;
+			os->driver->set_io_watch(os->object,
+							handle_async_io, os);
+		} else if (err < 0) {
+			os_set_response(obj, err);
+		}
 	}
 }
 
-- 
1.7.4.1

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Slawomir Bochenski]

On Wed, Oct 5, 2011 at 4:32 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
> OBEX_EV_REQ is the last thing that we are going to receive on PUT and
> this is always going to be delivered. As the service driver will start
> receiving data at or before its .put(), we can simply call flush() after
> that. This also makes flush() usable in PUTs without length header.

One thing I've forgotten to mention:

Additionally the return value from flush() now follows the usual
conventions - -EAGAIN is used to suspend request, other values less
than zero will set appropriate OBEX error code.

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Luiz Augusto von Dentz]

Hi Slawomir,

On Wed, Oct 5, 2011 at 5:32 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
> OBEX_EV_REQ is the last thing that we are going to receive on PUT and
> this is always going to be delivered. As the service driver will start
> receiving data at or before its .put(), we can simply call flush() after
> that. This also makes flush() usable in PUTs without length header.
> ---
>  src/obex.c |   28 ++++++++++------------------
>  1 files changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/src/obex.c b/src/obex.c
> index 38c5ca6..2cabadb 100644
> --- a/src/obex.c
> +++ b/src/obex.c
> @@ -625,11 +625,6 @@ write:
>                os->pending -= w;
>        }
>
> -       /* Flush on EOS */
> -       if (os->size != OBJECT_SIZE_UNKNOWN && os->size == os->offset &&
> -                                                       os->driver->flush)
> -               return os->driver->flush(os->object) > 0 ? -EAGAIN : 0;

By removing this it wont flush on EOS, so Im not sure why you removed
it since OBEX_EV_REQ is only generated for the request not for the
streams itself. Actually if OpenOBEX had an event for that represent
the FINAL bit that would solve the problem with unknown size, but for
gobex this is exposed directly in the callback so I wouldn't change
that now.

>        return 0;
>  }
>
> @@ -1140,19 +1135,16 @@ static void cmd_put(struct obex_session *os, obex_t *obex, obex_object_t *obj)
>                return;
>        }
>
> -       /* Check if there is a body and it is not empty (size > 0), otherwise
> -          openobex won't notify us with OBEX_EV_STREAMAVAIL and it gonna reply
> -          right away */
> -       if (os->size != 0)
> -               return;
> -
> -       /* Flush immediatly since there is nothing to write so the driver
> -          has a chance to do something before we reply */
> -       if (os->object && os->driver && os->driver->flush &&
> -                                       os->driver->flush(os->object) > 0) {
> -               OBEX_SuspendRequest(obex, obj);
> -               os->obj = obj;
> -               os->driver->set_io_watch(os->object, handle_async_io, os);
> +       if (os->object && os->driver && os->driver->flush) {
> +               err = os->driver->flush(os->object);
> +               if (err == -EAGAIN) {
> +                       OBEX_SuspendRequest(obex, obj);
> +                       os->obj = obj;
> +                       os->driver->set_io_watch(os->object,
> +                                                       handle_async_io, os);
> +               } else if (err < 0) {
> +                       os_set_response(obj, err);
> +               }

You got this wrong, the purpose of this code is to flush on end of
stream not in the beginning/put request, so if we have consecutive
puts the last packet should cause a flush so the driver can sync any
buffered data before close which needs to be immediately.

-- 
Luiz Augusto von Dentz

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Slawomir Bochenski]

> By removing this it wont flush on EOS, so Im not sure why you removed
> it since OBEX_EV_REQ is only generated for the request not for the
> streams itself. Actually if OpenOBEX had an event for that represent
> the FINAL bit that would solve the problem with unknown size, but for
> gobex this is exposed directly in the callback so I wouldn't change
> that now.
>
>
> You got this wrong, the purpose of this code is to flush on end of
> stream not in the beginning/put request, so if we have consecutive
> puts the last packet should cause a flush so the driver can sync any
> buffered data before close which needs to be immediately.

Again, OBEX_EV_REQ will be the very _last_ thing called. This code
flushes after all write()-s. After the final packet.


-- 
Slawomir Bochenski

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Slawomir Bochenski]

On Wed, Oct 5, 2011 at 5:17 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
>> By removing this it wont flush on EOS, so Im not sure why you removed
>> it since OBEX_EV_REQ is only generated for the request not for the
>> streams itself. Actually if OpenOBEX had an event for that represent
>> the FINAL bit that would solve the problem with unknown size, but for
>> gobex this is exposed directly in the callback so I wouldn't change
>> that now.
>>
>>
>> You got this wrong, the purpose of this code is to flush on end of
>> stream not in the beginning/put request, so if we have consecutive
>> puts the last packet should cause a flush so the driver can sync any
>> buffered data before close which needs to be immediately.
>
> Again, OBEX_EV_REQ will be the very _last_ thing called. This code
> flushes after all write()-s. After the final packet.

To be specific: the order of events from openobex are OBEX_EV_REQHINT,
OBEX_EV_REQCHECK, multiple OBEX_EV_STREAMAVAIL (as we are starting
streaming in REQHINT), and then OBEX_EV_REQ.
>
>
> --
> Slawomir Bochenski
>



-- 
Slawomir Bochenski

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Luiz Augusto von Dentz]

Hi Slawomir,

On Wed, Oct 5, 2011 at 6:20 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
> On Wed, Oct 5, 2011 at 5:17 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
>>> By removing this it wont flush on EOS, so Im not sure why you removed
>>> it since OBEX_EV_REQ is only generated for the request not for the
>>> streams itself. Actually if OpenOBEX had an event for that represent
>>> the FINAL bit that would solve the problem with unknown size, but for
>>> gobex this is exposed directly in the callback so I wouldn't change
>>> that now.
>>>
>>>
>>> You got this wrong, the purpose of this code is to flush on end of
>>> stream not in the beginning/put request, so if we have consecutive
>>> puts the last packet should cause a flush so the driver can sync any
>>> buffered data before close which needs to be immediately.
>>
>> Again, OBEX_EV_REQ will be the very _last_ thing called. This code
>> flushes after all write()-s. After the final packet.
>
> To be specific: the order of events from openobex are OBEX_EV_REQHINT,
> OBEX_EV_REQCHECK, multiple OBEX_EV_STREAMAVAIL (as we are starting
> streaming in REQHINT), and then OBEX_EV_REQ.

I don't think this is true, if it was then why would we call
service->put on the end of stream and I also never see this happening
in practice e.g. calling ftp_put in the end of stream.

-- 
Luiz Augusto von Dentz

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Slawomir Bochenski]

On Wed, Oct 5, 2011 at 5:38 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> I don't think this is true, if it was then why would we call
> service->put on the end of stream and I also never see this happening
> in practice e.g. calling ftp_put in the end of stream.

Actually, if you will add DBG call to filesystem_write() you will
clearly see, that ftp_put is called at the end of stream.

-- 
Slawomir Bochenski

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Luiz Augusto von Dentz]

Hi,

On Wed, Oct 5, 2011 at 7:12 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
> On Wed, Oct 5, 2011 at 5:38 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> I don't think this is true, if it was then why would we call
>> service->put on the end of stream and I also never see this happening
>> in practice e.g. calling ftp_put in the end of stream.
>
> Actually, if you will add DBG call to filesystem_write() you will
> clearly see, that ftp_put is called at the end of stream.

That is true only if the body is not bigger than the MTU, in other
words the body is not fragmented, otherwise it would be pointless to
check for name and folder only at the very end of the stream. I also
don't like the fact that OBEX_EV_REQ is generated in the end of the
first package, it is really confusing that a request event comes after
its data, but maybe there is a reason for that or the intention was
not that so we are not handling the event the way it was meant?

-- 
Luiz Augusto von Dentz

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Slawomir Bochenski]

On Wed, Oct 5, 2011 at 6:43 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Wed, Oct 5, 2011 at 7:12 PM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
>> On Wed, Oct 5, 2011 at 5:38 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> I don't think this is true, if it was then why would we call
>>> service->put on the end of stream and I also never see this happening
>>> in practice e.g. calling ftp_put in the end of stream.
>>
>> Actually, if you will add DBG call to filesystem_write() you will
>> clearly see, that ftp_put is called at the end of stream.
>
> That is true only if the body is not bigger than the MTU, in other
> words the body is not fragmented, otherwise it would be pointless to
> check for name and folder only at the very end of the stream.

False. You check the name and folder after OBEX_EV_REQCHECK, which was
specificaly added to allow earlier checks.

> I also
> don't like the fact that OBEX_EV_REQ is generated in the end of the
> first package, it is really confusing that a request event comes after
> its data, but maybe there is a reason for that or the intention was
> not that so we are not handling the event the way it was meant?

OBEX_EV_REQ is delivered when the remote party finished sending
request, not after first OBEX packet. This is the way it is intented
in OpenOBEX.

Please do check it or consult OpenOBEX code/documentation.

If you still do not agree, please to present an actual case when this
won't work. Please do try first with a transfer larger than the MTU
before assuming anything (maximum MTU is 64K, send a larger file),

-- 
Slawomir Bochenski

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Hendrik Sattler]

Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:

> Hi Slawomir,
>
> On Wed, Oct 5, 2011 at 6:20 PM, Slawomir Bochenski  
> <lkslawek@gmail.com> wrote:
>> On Wed, Oct 5, 2011 at 5:17 PM, Slawomir Bochenski  
>> <lkslawek@gmail.com> wrote:
>>>> By removing this it wont flush on EOS, so Im not sure why you removed
>>>> it since OBEX_EV_REQ is only generated for the request not for the
>>>> streams itself. Actually if OpenOBEX had an event for that represent
>>>> the FINAL bit that would solve the problem with unknown size, but for
>>>> gobex this is exposed directly in the callback so I wouldn't change
>>>> that now.
>>>>
>>>>
>>>> You got this wrong, the purpose of this code is to flush on end of
>>>> stream not in the beginning/put request, so if we have consecutive
>>>> puts the last packet should cause a flush so the driver can sync any
>>>> buffered data before close which needs to be immediately.
>>>
>>> Again, OBEX_EV_REQ will be the very _last_ thing called. This code
>>> flushes after all write()-s. After the final packet.
>>
>> To be specific: the order of events from openobex are OBEX_EV_REQHINT,
>> OBEX_EV_REQCHECK, multiple OBEX_EV_STREAMAVAIL (as we are starting
>> streaming in REQHINT), and then OBEX_EV_REQ.
>
> I don't think this is true, if it was then why would we call
> service->put on the end of stream and I also never see this happening
> in practice e.g. calling ftp_put in the end of stream.

The order is correct. Actually, with openobex-1.5 it may happen that  
you see one OBEX_EV_STREAMAVAIL before OBEX_EV_REQCHECK if a BODY  
header is included in the first packet, and no OBEX_EV_REQCHECK if the  
PUT has only one packet.

However, for current git (if a new version is ever going to be  
released), this order is strictly correct if the request is not  
aborted before being complete.

HS

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Luiz Augusto von Dentz]

Hi Hendrik,

On Thu, Oct 6, 2011 at 10:44 AM, Hendrik Sattler
<post@hendrik-sattler.de> wrote:
> Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
>
>> Hi Slawomir,
>>
>> On Wed, Oct 5, 2011 at 6:20 PM, Slawomir Bochenski <lkslawek@gmail.com>
>> wrote:
>>>
>>> On Wed, Oct 5, 2011 at 5:17 PM, Slawomir Bochenski <lkslawek@gmail.com>
>>> wrote:
>>>>>
>>>>> By removing this it wont flush on EOS, so Im not sure why you removed
>>>>> it since OBEX_EV_REQ is only generated for the request not for the
>>>>> streams itself. Actually if OpenOBEX had an event for that represent
>>>>> the FINAL bit that would solve the problem with unknown size, but for
>>>>> gobex this is exposed directly in the callback so I wouldn't change
>>>>> that now.
>>>>>
>>>>>
>>>>> You got this wrong, the purpose of this code is to flush on end of
>>>>> stream not in the beginning/put request, so if we have consecutive
>>>>> puts the last packet should cause a flush so the driver can sync any
>>>>> buffered data before close which needs to be immediately.
>>>>
>>>> Again, OBEX_EV_REQ will be the very _last_ thing called. This code
>>>> flushes after all write()-s. After the final packet.
>>>
>>> To be specific: the order of events from openobex are OBEX_EV_REQHINT,
>>> OBEX_EV_REQCHECK, multiple OBEX_EV_STREAMAVAIL (as we are starting
>>> streaming in REQHINT), and then OBEX_EV_REQ.
>>
>> I don't think this is true, if it was then why would we call
>> service->put on the end of stream and I also never see this happening
>> in practice e.g. calling ftp_put in the end of stream.
>
> The order is correct. Actually, with openobex-1.5 it may happen that you see
> one OBEX_EV_STREAMAVAIL before OBEX_EV_REQCHECK if a BODY header is included
> in the first packet, and no OBEX_EV_REQCHECK if the PUT has only one packet.
>
> However, for current git (if a new version is ever going to be released),
> this order is strictly correct if the request is not aborted before being
> complete.

Yep, so obexd is broken for quite some time, since we do check also on
OBEX_EV_REQ, but the documentation is not very clear about it since it
says:

/* An incoming request has arrived */

Also it doesn't help that OpenOBEX has tree events with similar
interpretation and probably because REQCHECK is not always generate we
had to have a proper check also on REQ too. Well I guess this is
another reason to move on and start porting the server side to gobex
which is much more mainloop friendly and less confusing is this
aspect.

-- 
Luiz Augusto von Dentz

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Slawomir Bochenski]

On Thu, Oct 6, 2011 at 10:17 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Hendrik,
>
> On Thu, Oct 6, 2011 at 10:44 AM, Hendrik Sattler
> <post@hendrik-sattler.de> wrote:
>> Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
>>
>>> Hi Slawomir,
>>>
>>> On Wed, Oct 5, 2011 at 6:20 PM, Slawomir Bochenski <lkslawek@gmail.com>
>>> wrote:
>>>>
>>>> On Wed, Oct 5, 2011 at 5:17 PM, Slawomir Bochenski <lkslawek@gmail.com>
>>>> wrote:
>>>>>>
>>>>>> By removing this it wont flush on EOS, so Im not sure why you removed
>>>>>> it since OBEX_EV_REQ is only generated for the request not for the
>>>>>> streams itself. Actually if OpenOBEX had an event for that represent
>>>>>> the FINAL bit that would solve the problem with unknown size, but for
>>>>>> gobex this is exposed directly in the callback so I wouldn't change
>>>>>> that now.
>>>>>>
>>>>>>
>>>>>> You got this wrong, the purpose of this code is to flush on end of
>>>>>> stream not in the beginning/put request, so if we have consecutive
>>>>>> puts the last packet should cause a flush so the driver can sync any
>>>>>> buffered data before close which needs to be immediately.
>>>>>
>>>>> Again, OBEX_EV_REQ will be the very _last_ thing called. This code
>>>>> flushes after all write()-s. After the final packet.
>>>>
>>>> To be specific: the order of events from openobex are OBEX_EV_REQHINT,
>>>> OBEX_EV_REQCHECK, multiple OBEX_EV_STREAMAVAIL (as we are starting
>>>> streaming in REQHINT), and then OBEX_EV_REQ.
>>>
>>> I don't think this is true, if it was then why would we call
>>> service->put on the end of stream and I also never see this happening
>>> in practice e.g. calling ftp_put in the end of stream.
>>
>> The order is correct. Actually, with openobex-1.5 it may happen that you see
>> one OBEX_EV_STREAMAVAIL before OBEX_EV_REQCHECK if a BODY header is included
>> in the first packet, and no OBEX_EV_REQCHECK if the PUT has only one packet.
>>
>> However, for current git (if a new version is ever going to be released),
>> this order is strictly correct if the request is not aborted before being
>> complete.
>
> Yep, so obexd is broken for quite some time, since we do check also on
> OBEX_EV_REQ, but the documentation is not very clear about it since it

obexd works as it should. The code is coping with this by doing a
buffering on behalf of a service driver if the service driver is not
using chkput. So from the service perspective it will be still put,
open, write, as the actual writing to mime driver is started at
obex_put_stream_start.

> says:
>
> /* An incoming request has arrived */
>
> Also it doesn't help that OpenOBEX has tree events with similar
> interpretation and probably because REQCHECK is not always generate we
> had to have a proper check also on REQ too. Well I guess this is
> another reason to move on and start porting the server side to gobex
> which is much more mainloop friendly and less confusing is this
> aspect.
>
> --
> 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
>

-- 
Slawomir Bochenski

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Luiz Augusto von Dentz]

Hi Slawomir,

On Thu, Oct 6, 2011 at 11:21 AM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
>
> obexd works as it should. The code is coping with this by doing a
> buffering on behalf of a service driver if the service driver is not
> using chkput. So from the service perspective it will be still put,
> open, write, as the actual writing to mime driver is started at
> obex_put_stream_start.

Except that we should not buffer more than MTU size, also this is
completely the opposite of what we should be doing which is to copy
buffers as little as possible. As for broken I mean the design is
broken since if the service does not implement .put obexd should
return not implemented right in the first packet, actually .put is
completely useless right now since it is basically as calling a .flush
but it does useless checks.

-- 
Luiz Augusto von Dentz

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Slawomir Bochenski]

On Thu, Oct 6, 2011 at 11:17 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Slawomir,
>
> On Thu, Oct 6, 2011 at 11:21 AM, Slawomir Bochenski <lkslawek@gmail.com> wrote:
>>
>> obexd works as it should. The code is coping with this by doing a
>> buffering on behalf of a service driver if the service driver is not
>> using chkput. So from the service perspective it will be still put,
>> open, write, as the actual writing to mime driver is started at
>> obex_put_stream_start.
>
> Except that we should not buffer more than MTU size, also this is
> completely the opposite of what we should be doing which is to copy
> buffers as little as possible. As for broken I mean the design is
> broken since if the service does not implement .put obexd should
> return not implemented right in the first packet, actually .put is
> completely useless right now since it is basically as calling a .flush
> but it does useless checks.

>From my point of view as a person external to the project, developing
the new service driver, I assumed that the policy that allows making
drivers with both .chkput/.put and .put only has been provided on some
purpose. Even if this purpose doesn't seem to give any gain (well,
except maybe when one would like to access full buffered request from
a single write) and can lead to OOM in case of a service that doesn't
do obex_put_stream_start right in .chkput. Thus I didn't think of .put
as something useless.

If no, then the problem is not in the OpenOBEX itself, but in the way
it is used in obexd. OBEX_EV_REQCHECK is the event that is to be
delivered after the first packet. As Hendrik mentioned, it can be
omitted if there was only one packet, but obexd deals with it -
check_put() is also called from cmd_put() (called on OBEX_EV_REQ),
when the request haven't been yet "checked".

This could be done better by getting rid of .chkput and calling .put
on OBEX_EV_REQCHECK (or EV_REQ if no previous EV_REQCHECK) and .flush
always on EV_REQ.

And you are right that unfortunately checking for .put existence is
done too late, so the request will be fully received before responding
with "not implemented". Otherwise things are quite functional.

But well, now with the obexd going to be rewritten to use gobex, I
suppose these things do not matter any longer.

-- 
Slawomir Bochenski

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Hendrik Sattler]

Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
> On Thu, Oct 6, 2011 at 10:44 AM, Hendrik Sattler
> <post@hendrik-sattler.de> wrote:
>> The order is correct. Actually, with openobex-1.5 it may happen that you see
>> one OBEX_EV_STREAMAVAIL before OBEX_EV_REQCHECK if a BODY header is included
>> in the first packet, and no OBEX_EV_REQCHECK if the PUT has only one packet.
>>
>> However, for current git (if a new version is ever going to be released),
>> this order is strictly correct if the request is not aborted before being
>> complete.
>
> Yep, so obexd is broken for quite some time, since we do check also on
> OBEX_EV_REQ, but the documentation is not very clear about it since it
> says:
>
> /* An incoming request has arrived */
>
> Also it doesn't help that OpenOBEX has tree events with similar
> interpretation and probably because REQCHECK is not always generate we
> had to have a proper check also on REQ too.

Ask Marcel why there is no new release of OpenOBEX! This could have  
been fixed for months.

> Well I guess this is another reason to move on and start porting the server
> side to gobex which is much more mainloop friendly and less confusing is this
> aspect.

s/mainloop/glib/

HS

Re: [PATCH obexd 4/4] Simplify code for calling mime driver flush()

[Johan Hedberg]

Hi Slawek,

On Wed, Oct 05, 2011, Slawomir Bochenski wrote:
> OBEX_EV_REQ is the last thing that we are going to receive on PUT and
> this is always going to be delivered. As the service driver will start
> receiving data at or before its .put(), we can simply call flush() after
> that. This also makes flush() usable in PUTs without length header.
> ---
>  src/obex.c |   28 ++++++++++------------------
>  1 files changed, 10 insertions(+), 18 deletions(-)

This fourth patch has now also been applied. Thanks.

Johan

Re: [PATCH obexd 1/4] Reverse order of calls in os_reset_session

[Johan Hedberg]

Hi Slawek,

On Wed, Oct 05, 2011, Slawomir Bochenski wrote:
> This replaces:
> service->get/put, mime->open, ..., service->reset, mime->close
> logic, with a more appropriate:
> service->get/put, mime->open, ..., mime->close, service->reset
> ---
>  src/obex.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)

Patches 1-3 have been applied. Thanks.

Johan