Hendrik Sattler | 6 Dec 20:46 2010
Picon

Please review these patches

Hi,

I have some patches in my queue. Since Johan said that I should merge more
often... ;) Comments are welcome.

The following changes since commit 712971264d788e87200685fb4a36ab75f51d84ef:

  Fix OBEX_CancelRequest for server mode (2010-10-15 14:07:20 +0300)

are available in the git repository at:
  http://git.gitorious.org/openobex/mainline.git for-mainline

Hendrik Sattler (13):
      Make argument of buf_total_size() const
      API documentation updates
      Strip second argument of obex_object_setcmd()
      Remove STATE_START
      Handle multiple messages in RX message queue
      Return correct return value for USB-1.x write timeout
      Allow custom transport to insert whole packets on read()
      Add support for single response mode
      Reformat debug dump output
      Make obex_object_send() easier to understand
      Add SRM header ID definitions
      Add support for SRM flags
      Use fast path for rejected packets

 include/openobex/obex.h       |    3 +
 include/openobex/obex_const.h |   19 +++-
 lib/customtrans.c             |   22 ++++-
(Continue reading)

Johan Hedberg | 8 Dec 11:05 2010
Picon

Re: Please review these patches

Hi Hendrik,

On Mon, Dec 06, 2010, Hendrik Sattler wrote:
> I have some patches in my queue. Since Johan said that I should merge more
> often... ;) Comments are welcome.
> 
> The following changes since commit 712971264d788e87200685fb4a36ab75f51d84ef:
> 
>   Fix OBEX_CancelRequest for server mode (2010-10-15 14:07:20 +0300)
> 
> are available in the git repository at:
>   http://git.gitorious.org/openobex/mainline.git for-mainline
> 
> Hendrik Sattler (13):
>       Make argument of buf_total_size() const
>       API documentation updates
>       Strip second argument of obex_object_setcmd()
>       Remove STATE_START
>       Handle multiple messages in RX message queue
>       Return correct return value for USB-1.x write timeout
>       Allow custom transport to insert whole packets on read()
>       Add support for single response mode
>       Reformat debug dump output
>       Make obex_object_send() easier to understand
>       Add SRM header ID definitions
>       Add support for SRM flags
>       Use fast path for rejected packets
> 
>  include/openobex/obex.h       |    3 +
>  include/openobex/obex_const.h |   19 +++-
(Continue reading)

Hendrik Sattler | 8 Dec 12:33 2010
Picon

Re: Please review these patches

Zitat von "Johan Hedberg" <johan.hedberg <at> gmail.com>:
> The patches have been pushed upstream. I didn't actually test them in
> practice, so I hope you've done that thoroughly ;)

I'm sure that there are some bugs left, I am not perfect, after all :)
I tested with my obexpushd and obexftp, once with my USB AT modem  
emulation custom transport (obexftp -t /dev/ttyACM0) and with local  
TCP connection (obexftp -n 127.0.0.1). I haven't figured out how to  
get a Bluetooth local loop (that actually uses the bluetooth stack).  
Also using dummy_hcd.ko with g_serial use_acm=0 use_obex=1 randomly  
crashes, so I am not using it.

Thanks to Hui Li for showing how to use SRM :)
Maybe he can provide a patch for a native OBEX-over-L2CAP at least for  
the Linux case (then I can also look up how to do this for Windows  
case if possible at all).

Note: with those patches, an OBEX server _always_ gets a EV_REQCHECK.  
I see this as necessary for being fully consistent as the app usually  
doesn't care if an event is single-packet or multi-packet.  
Additionally, setting the response to CONTINUE/SUCCESS is now  
_required_. It saves a lot of work for requests that are not wanted  
and makes it a lot easier for the application to not do anything wrong.

I will do some documentation updates to tighten the description of all  
events and what to do there.

> I also pushed a code/coding style cleanup patch on top of these, but to
> be fair a lot of the cleanup (maybe even most) wasn't related to issues
> introduced by your patches but existed in the code from before. While
(Continue reading)

Hendrik Sattler | 8 Dec 12:41 2010
Picon

Re: Please review these patches

Zitat von "Hendrik Sattler" <post <at> hendrik-sattler.de>:
> Maybe he can provide a patch for a native OBEX-over-L2CAP at least for
> the Linux case (then I can also look up how to do this for Windows
> case if possible at all).

Means, I do not intend to write any code for that as I cannot test  
that at all.

> Note: with those patches, an OBEX server _always_ gets a EV_REQCHECK.
> I see this as necessary for being fully consistent as the app usually
> doesn't care if an event is single-packet or multi-packet.
> Additionally, setting the response to CONTINUE/SUCCESS is now
> _required_.
    ^^^^
at the time of EV_REQHINT.
A negative response code was previously ignored until after  
EV_REQCHECK (and for single-packet requests even later).
I speaking about server here, clients don't get those events.

HS

------------------------------------------------------------------------------
What happens now with your Lotus Notes apps - do you make another costly 
upgrade, or settle for being marooned without product support? Time to move
off Lotus Notes and onto the cloud with Force.com, apps are easier to build,
use, and manage than apps on traditional platforms. Sign up for the Lotus 
Notes Migration Kit to learn more. http://p.sf.net/sfu/salesforce-d2d
Johan Hedberg | 8 Dec 12:55 2010
Picon

Re: Please review these patches

Hi Hendrik,

On Wed, Dec 08, 2010, Hendrik Sattler wrote:
> Zitat von "Johan Hedberg" <johan.hedberg <at> gmail.com>:
> > The patches have been pushed upstream. I didn't actually test them in
> > practice, so I hope you've done that thoroughly ;)
> 
> I'm sure that there are some bugs left, I am not perfect, after all :)
> I tested with my obexpushd and obexftp, once with my USB AT modem  
> emulation custom transport (obexftp -t /dev/ttyACM0) and with local  
> TCP connection (obexftp -n 127.0.0.1). I haven't figured out how to  
> get a Bluetooth local loop (that actually uses the bluetooth stack).  
> Also using dummy_hcd.ko with g_serial use_acm=0 use_obex=1 randomly  
> crashes, so I am not using it.

We're gonna do a test package based on current git and run our usual
test set on it (the one we use internally at Nokia/MeeGo). obexd and
obex-client (also from the obexd package) are used for those. Hopefully
we'll catch any regressions.

> > I also pushed a code/coding style cleanup patch on top of these, but to
> > be fair a lot of the cleanup (maybe even most) wasn't related to issues
> > introduced by your patches but existed in the code from before. While
> > going through the code I noticed that it could still use quite a lot of
> > refactoring in order to get rid of deeply nested if-statements, etc.
> > It's by far the most common cause of it being almost impossible to
> > adhere to the "max 79 columns" rule everywhere.
> 
> I don't like the style but I don't care enough, either.

(Continue reading)

Hendrik Sattler | 8 Dec 13:31 2010
Picon

Re: Please review these patches

Hi Johan,

Zitat von "Johan Hedberg" <johan.hedberg <at> gmail.com>:
> On Wed, Dec 08, 2010, Hendrik Sattler wrote:
>> Zitat von "Johan Hedberg" <johan.hedberg <at> gmail.com>:
>> > The patches have been pushed upstream. I didn't actually test them in
>> > practice, so I hope you've done that thoroughly ;)
>>
>> I'm sure that there are some bugs left, I am not perfect, after all :)
>> I tested with my obexpushd and obexftp, once with my USB AT modem
>> emulation custom transport (obexftp -t /dev/ttyACM0) and with local
>> TCP connection (obexftp -n 127.0.0.1). I haven't figured out how to
>> get a Bluetooth local loop (that actually uses the bluetooth stack).
>> Also using dummy_hcd.ko with g_serial use_acm=0 use_obex=1 randomly
>> crashes, so I am not using it.
>
> We're gonna do a test package based on current git and run our usual
> test set on it (the one we use internally at Nokia/MeeGo). obexd and
> obex-client (also from the obexd package) are used for those. Hopefully
> we'll catch any regressions.

Great :)

> I hope you at least agree with changes like:
>
> * not having unnecessary whitespace at the end of lines (in fact your
>   patches introduces some)

Sorry, then. Sometimes, I forget to check with the emacs whitespace-mode.

(Continue reading)

Luiz Augusto von Dentz | 8 Dec 13:38 2010
Picon

Re: Please review these patches

Hi,

On Wed, Dec 8, 2010 at 1:33 PM, Hendrik Sattler <post <at> hendrik-sattler.de> wrote:
> Additionally, setting the response to CONTINUE/SUCCESS is now
> _required_. It saves a lot of work for requests that are not wanted
> and makes it a lot easier for the application to not do anything wrong.

Does it means that if the application don't set a response it will
have the same effect as a suspend? If yes, does it means
suspend/waiting on SRM?

--

-- 
Luiz Augusto von Dentz
Computer Engineer

------------------------------------------------------------------------------
What happens now with your Lotus Notes apps - do you make another costly 
upgrade, or settle for being marooned without product support? Time to move
off Lotus Notes and onto the cloud with Force.com, apps are easier to build,
use, and manage than apps on traditional platforms. Sign up for the Lotus 
Notes Migration Kit to learn more. http://p.sf.net/sfu/salesforce-d2d
Hendrik Sattler | 8 Dec 14:48 2010
Picon

Re: Please review these patches

Zitat von "Luiz Augusto von Dentz" <luiz.dentz <at> gmail.com>:
> On Wed, Dec 8, 2010 at 1:33 PM, Hendrik Sattler  
> <post <at> hendrik-sattler.de> wrote:
>> Additionally, setting the response to CONTINUE/SUCCESS is now
>> _required_. It saves a lot of work for requests that are not wanted
>> and makes it a lot easier for the application to not do anything wrong.
>
> Does it means that if the application don't set a response it will
> have the same effect as a suspend? If yes, does it means
> suspend/waiting on SRM?

No. At this stage, the application does only know the type of request,  
nothing else. So it should only decide if it can support it or not  
(based on implementation and current target). Suspend doesn't make  
sense at that point.
Not setting any response means (as previously) that the request type  
is not implemented. Previously, the request was decoded anyway and  
other events were raised. This is not the case anymore.
We can also invert that behaviour, using CONTINUE/SUCCESS as default.  
This would mean that the application only has NACK requests, not ACK  
them. Previouly, it was not really well-defined, so most application  
probably do both.

The application is called with EV_REQCHECK shortly after EV_REQHINT  
with decoded information about the request. At that point, you can  
even decide to receive the data as stream (which gives you additional  
events after returning from the event, suspended or not) or if you  
want it buffered in the lib. You can initiate the suspend at that  
point or on the next call of EV_PROGRESS. The EV_PROGRESS now comes  
_before_ sending the CONTINUE response (thus having the same meaning  
(Continue reading)

Luiz Augusto von Dentz | 9 Dec 09:46 2010
Picon

[PATCH] Fix not resetting state to idle when canceling a request on server mode

From: Luiz Augusto von Dentz <luiz.dentz-von <at> nokia.com>

Also deliver ABORT signal so the user application can free whatever
resource being use to handle the request.
---
 lib/obex_main.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/lib/obex_main.c b/lib/obex_main.c
index 26bf04b..0cadb2f 100644
--- a/lib/obex_main.c
+++ b/lib/obex_main.c
 <at>  <at>  -446,7 +446,10  <at>  <at>  int obex_cancelrequest(obex_t *self, int nice)
 		obex_object_delete(object);

 		self->object->abort = TRUE;
-		self->state = STATE_REC;
+		self->state = self->mode == MODE_SRV ? STATE_IDLE : STATE_REC;
+
+		/* Deliver event will delete the object */
+		obex_deliver_event(self, OBEX_EV_ABORT, 0, 0, TRUE);

 		return 0;
 	}
--

-- 
1.7.1

------------------------------------------------------------------------------
This SF Dev2Dev email is sponsored by:

(Continue reading)

Luiz Augusto von Dentz | 9 Dec 11:06 2010
Picon

[PATCH v2] Fix not resetting state to idle when canceling a request on server mode

From: Luiz Augusto von Dentz <luiz.dentz-von <at> nokia.com>

Also deliver ABORT signal so the user application can free whatever
resource being use to handle the request.
---
 lib/obex_main.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/lib/obex_main.c b/lib/obex_main.c
index 26bf04b..aff3aaf 100644
--- a/lib/obex_main.c
+++ b/lib/obex_main.c
 <at>  <at>  -446,7 +446,11  <at>  <at>  int obex_cancelrequest(obex_t *self, int nice)
 		obex_object_delete(object);

 		self->object->abort = TRUE;
-		self->state = STATE_REC;
+		self->state = self->mode == MODE_SRV ? STATE_IDLE : STATE_REC;
+
+		if (self->state == STATE_IDLE)
+			/* Deliver event will delete the object */
+			obex_deliver_event(self, OBEX_EV_ABORT, 0, 0, TRUE);

 		return 0;
 	}
--

-- 
1.7.1

------------------------------------------------------------------------------
This SF Dev2Dev email is sponsored by:
(Continue reading)


Gmane