patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6250, MSG_MORE flag for send

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6250: MSG_MORE flag for send

Submitted by:  Marc Chaland <chaland>
Submitted on:  Mon 29 Oct 2007 08:59:07 AM UTC  
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  Frédéric Bernon <fbernon> Open/Closed:  Closed
Planned Release:  1.3.0

( Jump to the original submission)

Thu 01 Nov 2007 07:36:16 PM UTC, comment #23: 

About the assert, if we do it here, we should do it to any function using a bit-field flag, so, I'm not in favor of add it (but no against).

About comments, I will add it...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 01 Nov 2007 06:47:16 PM UTC, comment #22: 

What about adding a check for known apiflags à là

#ifdef TCP_OUTPUT_DEBUG
LWIP_ASSERT("Unknown apiflags", ((apiflags & ~(TCP_WRITE_FLAG_COPY | TCP_WRITE_FLAG_MORE)) == 0);
#endif /* TCP_OUTPUT_DEBUG */

?

Also, a comment above TCP_WRITE_FLAG_COPY and TCP_WRITE_FLAG_MORE that these are usable as apiflags for tcp_write and tcp_enqueue would be good ;-)

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 01 Nov 2007 05:39:12 PM UTC, comment #21: 

Ok, patch is integrated. Thank you Marc for this new feature.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 01 Nov 2007 12:55:17 PM UTC, comment #20: 

Apologies for not entering the debate earlier, but I think you've all come to the right decisions without me, so please close as fixed once the patch goes in.

Kieran Mansley <kieranm>
Project Member
Thu 01 Nov 2007 12:28:00 PM UTC, comment #19: 

Looks fine!

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 30 Oct 2007 06:54:52 PM UTC, comment #18: 

Fine by me, and thankyou to Marc for his good work on it.

Jonathan Larmour <jifl>
Project Member
Tue 30 Oct 2007 06:45:03 PM UTC, comment #17: 

If there is no objection, I will integrate the patch in the CVS HEAD this week.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 30 Oct 2007 05:00:31 PM UTC, comment #16: 

Patch corrected with TCP_WRITE_FLAG and sockets.h reverted.
I got an idea to deal with libc headers compatibility. I'll try and test it.

(file #14259)

Marc Chaland <chaland>
Tue 30 Oct 2007 03:16:10 PM UTC, comment #15: 

TCP_WRITE_FLAG_xxx works for me.

Jonathan Larmour <jifl>
Project Member
Tue 30 Oct 2007 01:08:54 PM UTC, comment #14: 

For the "defines" problem, I'm in flavor to not treat that in this item. In a first time, Marc, are you agree we put MSG_MORE=0x10 in lwip's sockets.h (even if you change them in your local copy)?

Else about the name TCP_WRITE_FLAG_xxx ?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 30 Oct 2007 12:34:33 PM UTC, comment #13: 

To be clear, I think what this means is that for those in Marc's position, you should separate out functions that use lwIP socket includes into different modules from those that use uclibc socket includes.

As for the name of API_FLAG_xxx etc., the only alternative I could think of is something like: LWIP_WRITE_FLAG_xxxx (since these flags are specific to writes). It seems better, but not much. Just "WRITE_FLAG_xxxx" (no LWIP_) seems like it might risk clashes with application names.

But I'm not suggesting the patch is updated unless someone else thinks it is a better name.

Jonathan Larmour <jifl>
Project Member
Tue 30 Oct 2007 12:29:16 PM UTC, comment #12: 

There won't be standard values.

I think anyone who tries to mix values and defines from one TCP/IP stack implementation with values from another is asking for trouble. If we fix this one case, there is bound to be something else. I say just leave the existing values.

Jonathan Larmour <jifl>
Project Member
Tue 30 Oct 2007 12:14:20 PM UTC, comment #11: 

I just got the last uclibc from http://www.uclibc.org. I think the problem is not only MSG_ flags, but all sockets.h: I see lot of things which could cause conflicts in uclibc's "sys/socket.h" (and "bits/socket.h"), with lwip's "sockets.h".

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 30 Oct 2007 11:43:55 AM UTC, comment #10: 

I don't think there is any standart values for that (but if you have some reference docs to send us, please, do it). But why uclibc  headers define MSG_ flags? If they are only used by sockets API, why don't simply remove them from uclibc? That why I would like to know which other uclibc functions use them. You could also include them your "uclibc.h" file in your cc.h file. Like this, the #if (!defined...) will work. This is the same problem that ERRNO by example, or FIONREAD and FIONBIO (and some others)...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 30 Oct 2007 11:33:36 AM UTC, comment #9: 

If an application uses uclibc headers for example, it will call send() with MSG_MORE=0x8000. If into lwip library, MSG_MORE=0x10, library will never detect that MSG_MORE flag has been set.
As lwip is not compiled with uclibc, I don't think using ifdef is a good idea. I think lwip should use more "standard" flags.

Marc

Marc Chaland <chaland>
Tue 30 Oct 2007 10:49:45 AM UTC, comment #8: 

The problem is to know if these MSG_ defines are always used only for sockets or not. But as long they are defined like "bit-fields" on a u16_t, I don't think there will be any problems inside lwIP. Perhaps I'm wrong...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 30 Oct 2007 10:34:49 AM UTC, comment #7: 

> #if ((!defined(MSG_PEEK)) && (!defined(MSG_WAITALL)) &&
> (!defined(MSG_OOB)) && (!defined(MSG_DONTWAIT)))


I think that's a bad idea. That way, you won't even get a warning when another header defines these constants to a different value - but the software doesn't work!

Better
a) to leave it like it is and let the user sort it out or
b) rename the constants (which isn't really portable...)

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 30 Oct 2007 09:09:28 AM UTC, comment #6: 

>1. For sockets.h, I changed values of MSG_*. In fact, many client application that I port to L4 use uclibc headers which are incompatible with sockets.h one. I already tried to remove uclibc header, but it provides some function that are not implemented into lwip library. I saw that all libc that I know use those values. So I think its better to reuse them ? :)


I'm not sure there is a standard for these values. If another user got the same build problem than you, but with different values, what should we do? Better to change them, I think we could add something like :

#if ((!defined(MSG_PEEK)) && (!defined(MSG_WAITALL)) && (!defined(MSG_OOB)) && (!defined(MSG_DONTWAIT)))
/* Flags we can use with send and recv. */
#define MSG_PEEK       0x01    /* Peeks at an incoming message */
#define MSG_WAITALL    0x02    /* Requests that the function block until the full amount of data requested can be returned */
#define MSG_OOB        0x04    /* Requests out-of-band data. The significance and semantics of out-of-band data are protocol-specific */
#define MSG_DONTWAIT   0x08    /* Nonblocking i/o for this operation only */
#endif

It's already done for some others parameters. Just for my information, what other uclibc functions used these MSG_ defined?

>2. I changed the flags in sockets.c


ok

>3. I changed the comments in tcp_out.c and api_lib.c


ok

>4. For API_FLAG_xxx, no idea too


Perhaps some others developers can suggest better names? Wait and see...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 30 Oct 2007 08:52:46 AM UTC, comment #5: 

1. For sockets.h, I changed values of MSG_*. In fact, many client application that I port to L4 use uclibc headers which are incompatible with sockets.h one. I already tried to remove uclibc header, but it provides some function that are not implemented into lwip library.
I saw that all libc that I know use those values. So I think its better to reuse them ? :)
2. I changed the flags in sockets.c
3. I changed the comments in tcp_out.c and api_lib.c
4. For API_FLAG_xxx, no idea too

(file #14255)

Marc Chaland <chaland>
Mon 29 Oct 2007 08:56:50 PM UTC, comment #4: 

Yes, last patch seems good, except some minor things:

- I don't understand your changes in sockets.h

- I think it's better to use NETCONN_COPY in sockets.c (and not directly API_FLAG_COPY)

- some comments don't seem to be explicit enought : "@param apiflags 2 if PSH flag should be set on last segment, 1 if data must be copied, 0 if data is non-volatile and can be referenced"

Last, I think we could find some better names for API_FLAG_xxx and for apiflags (but, I don't have an idea now).

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 29 Oct 2007 05:45:52 PM UTC, comment #3: 

I changed the patch (lwip-071029.patch) to see if I well understood what you mean.

Marc

(file #14247)

Marc Chaland <chaland>
Mon 29 Oct 2007 04:30:49 PM UTC, comment #2: 

[ Moving this discussion to patch from list ]

Frédéric BERNON wrote:

>
> Without adding a new parameter, since the existing "copy"
> parameter seems to be forwarded from sockets.c to tcp_out.c
> (in tcp_enqueue), why don't use it? We can rename it apiflags -
> or something else - (since flags is usein tcp_enqueue like "tcp
> flags"). I don't have study all case, but msg_more is an option
> "per 'write' call", like the "copy" option.


I think that's a very good idea.

Marc CHALAND wrote:

> This was my first idea. I didn't want to disturb eventual
> special use of this byte ?


I think this counts as special use :-).

> However, this require change of tcp_write and so on
> functions...


As does your proposed patch, but here we are only changing the name of 'copy', which obviously doesn't change code size. Only tcp_enqueue actually has to mask off bits.

I think this works right up to netconn_write() too, so that would change from 'copy' to 'apiflags' too. In the apiflags, 1 would mean "copy", thus preserving existing API behaviour. So we even get away with no API backward compatibility problem.

Jonathan Larmour <jifl>
Project Member
Mon 29 Oct 2007 09:09:10 AM UTC, comment #1: 

Some informations from the mailing list about this patch :

http://lists.nongnu.org/archive/html/lwip-devel/2007-10/msg00082.html

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 29 Oct 2007 08:59:07 AM UTC, original submission:  

To port lwip on L4, I need a functionnality which seems to me interesting for 1.3.0 version. This little patch has been done and tested on 1.2.0. I did a port to current CVS version. This functionnality allows to late the TCP PUSH flag.

Is it better to use PCB to avoid so many changes ?
copy byte in api_msg should be changed to apiflag ?

Marc

Marc Chaland <chaland>

 

Attached Files
file #14259:  lwip-071030-1.patch added by chaland (9KiB - application/octet-stream)
file #14255:  lwip-071030.patch added by chaland (10KiB - application/octet-stream)
file #14247:  lwip-071029.patch added by chaland (10KiB - application/octet-stream)
file #14242:  lwip-071026.patch added by chaland (8KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by chaland (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-11-01 fbernon StatusNone => Done
        Assigned toNone => fbernon
        Open/ClosedOpen => Closed
        Planned ReleaseNone => 1.3.0
    2007-10-30 chaland Attached File- => Added lwip-071030-1.patch, #14259
    2007-10-30 chaland Attached File- => Added lwip-071030.patch, #14255
    2007-10-29 chaland Attached File- => Added lwip-071029.patch, #14247
    2007-10-29 chaland Attached File- => Added lwip-071026.patch, #14242

    Back to the top


    Powered by Savane 3.5