patchlwIP - A Lightweight TCP/IP stack - Patches: patch #5865, Fix missing lwip directory in...

 
 

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

patch #5865: Fix missing lwip directory in socket.h including path

Submitted by:  Tai-hwa Liang <atliang>
Submitted on:  Thu 12 Apr 2007 08:50:20 AM UTC  
 
Category:  None Priority:  3 - Low
Status:  Done Privacy:  Public
Assigned to:  Kieran Mansley <kieranm> Open/Closed:  Closed
Planned Release:  None

( Jump to the original submission)

Mon 21 May 2007 04:33:18 PM UTC, comment #19: 

Kieran, I think you can close it...

Frédéric Bernon <fbernon>
Project Member
Mon 14 May 2007 12:03:32 PM UTC, comment #18: 

Frédéric has checked that in, already.

I don't think that ip_addr.h need opt.h. There's only the #ifdef PACK_STRUCT_USE_INCLUDES, which (I think) has to be define in cc.h (which is included by arch.h). It seems more like ip_addr.h would neet inet.h, since it uses hton-functions in some defines. But including it in sockets.h is enough, I think.

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 14 May 2007 10:47:15 AM UTC, comment #17: 

I'm with Frederic that it should be included. sockets.h is effectively the API file, and it should be defining htonl etc.

Also, I don't think it's important whether opt.h comes before ip_addr.h. If ip_addr.h needs something from opt.h, it should be including it itself. It doesn't seem to though, unless the port itself does it in lwip/arch.h.

Jonathan Larmour <jifl>
Project Member
Mon 14 May 2007 08:26:33 AM UTC, comment #16: 

about include inet.h, I'm agree it's not directly use by sockets functions (but this is the same thing for ip_addr.h). In most of application ports I do, only sockets.h was necessary. From memory, sockets.h is used to make a "more" portable code (it is defined in most of ip stacks), because, even in unix, the real file to call is not sockets.h, but socket.h (exactly, sys/socket.h, see http://www.opengroup.org/onlinepubs/007908799/xns/syssocket.h.html). Most of time, one include the other one.

>But I also don't see it that strict, that was just my reason not to include it. Go ahead and include it if you want. Same thing for moving opt.h before ip_addr.h


But if it's ok for you, I will check in like that...

Frédéric Bernon <fbernon>
Project Member
Mon 14 May 2007 07:56:49 AM UTC, comment #15: 

About the hton-warnings: I know the warnings appear now, and we could get rid of them by including inet.h in sockets.h. But strictly, sockets.h does not need the hton-functions and maybe the user wants to use others than the lwIP-internal ones. Coding on linux, you also have to include inet.h (or how is it called? I can't remember...) if you want to use hton-functions...

But I also don't see it that strict, that was just my reason not to include it. Go ahead and include it if you want. Same thing for moving opt.h before ip_addr.h

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 14 May 2007 07:48:04 AM UTC, comment #14: 

Hmm, seems with removing tcp.h from sockets.h, I got now lot of warnings like :

warning: function "htonl" declared implicitly
warning: function "inet_addr" declared implicitly
...

It seems this problem was masked before because include chain was :

tcp.h
    ip.h
        netif.h
            inet.h

Because ip_addr.h is already in sockets.h, I propose to add inet.h.       
Question: why in sockets.h, opt.h is after ip_addr.h include ?

Frédéric Bernon <fbernon>
Project Member
Sun 13 May 2007 02:34:25 PM UTC, comment #13: 

>Yeah, I think I must have read LWIP_TCP_KEEPALIVE as TCP_KEEPALIVE in tcp.c


In fact, initially, I agreed with you since I searched all files for 'TCP_KEEPALIVE'. The I saw I hadn't turned on case sensitivity and the 'whole word' button ;-)

OK, since you were the only one speaking against it, I've check this in.

I won't close it, though, since it's not assigned to me!

Simon Goldschmidt <goldsimon>
Project Administrator
Sun 13 May 2007 02:19:30 PM UTC, comment #12: 

Yeah, I think I must have read LWIP_TCP_KEEPALIVE as TCP_KEEPALIVE in tcp.c. In that case I do still agree with Simon after all ;). The patch appears sane - go for it as far as I'm concerned.

Jonathan Larmour <jifl>
Project Member
Sun 13 May 2007 02:06:14 PM UTC, comment #11: 

>While I initially agreed with Simon, I'm not sure now, given that I notice that both TCP_KEEPALIVE and TCP_MAXIDLE are used in the lwIP core code.


Correct me if I'm wrong, but TCP_KEEPALIVE is not used in the core code and TCP_MAXIDLE shouldn't be moved to sockets.h since it is not used in sockets.c. I've prepared a patch moving the #include "lwip/tcp.h" from sockets.h to sockets.c and moving TCP set/getsockopt options from tcp.h to sockets.h since they are only used in sockets.c! Those are the following:

- TCP_NODELAY
- TCP_KEEPALIVE
- TCP_KEEPIDLE
- TCP_KEEPINTVL
- TCP_KEEPCNT

This patch removes socket application dependancy from core headers.

(file #12763)

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 20 Apr 2007 01:59:06 PM UTC, comment #10: 

While I initially agreed with Simon, I'm not sure now, given that I notice that both TCP_KEEPALIVE and TCP_MAXIDLE are used in the lwIP core code. Given that, they should be in a core header. And I think the options should live in the same headerfile together. Some different defines could separate the abstractions without penalty.

Indeed it's a bit dubious that lwip_setsockopt() tweaks the PCBs directly with no locking, but that's a separate issue.

Jonathan Larmour <jifl>
Project Member
Fri 20 Apr 2007 08:35:20 AM UTC, comment #9: 

To protect the point of view for the current implementation: linux does it the same way (defining TCP_NODELAY etc. in tcp.h and thus exporting all tcp structs with socket.h), but I still think it would be better to separate sockets (and their options) from internal implementation.

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 20 Apr 2007 08:00:58 AM UTC, comment #8: 

I have to say I agree with Simon on this.  I'll investigate how these options could be moved around to prevent the need for this dependency

Kieran Mansley <kieranm>
Project MemberIn charge of this item.
Fri 20 Apr 2007 06:29:07 AM UTC, comment #7: 

I just think it's strange that the Options for level IPPROTO_IP are included in sockets.h while the tcp options are in tcp.h, although they have nothing to do with our tcp implementation but are a arguments to a function in sockets.c

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 20 Apr 2007 06:20:39 AM UTC, comment #6: 

Hi,

I think that the patch is good, and that tcp.h must stay here to reach some options defines like TCP_NODELAY, TCP_KEEPALIVE, .... If yo really want to remove it, think to add all necessary defines in sockets.h.

Note that because some others include like opt.h and ip_addr.h are necessary, I don't understand why tcp.h will cause a problem...

And you will have to maintain a compatible table like SO_ options (in sockets.h) and SOF_ options (in ip.h), like this example :
#define  SO_BROADCAST   0x0020 /* permit sending of broadcast msgs */
#define SOF_BROADCAST (u16_t)0x0020U    /* permit sending of broadcast msgs */

And to my point of view, it's not very nice...

Frédéric Bernon <fbernon>
Project Member
Fri 20 Apr 2007 02:07:20 AM UTC, comment #5: 

I guess that one of the reason to put TCP_NODELAY inside tcp.h is to keep all user-settable options together, as commented in lwip/tcp.h:

/*

  • User-settable options (used with setsockopt).

*/
#define TCP_NODELAY    0x01    /* don't delay send to coalesce packets */
#define TCP_KEEPALIVE  0x02    /* send KEEPALIVE probes when idle for pcb->keep_
idle milliseconds */

#define TCP_KEEPIDLE   0x03    /* set pcb->keep_idle  - Same as TCP_KEEPALIVE, b
ut use seconds for get/setsockopt*/
#define TCP_KEEPINTVL  0x04    /* set pcb->keep_intvl - Use seconds for get/sets
ockopt */
#define TCP_KEEPCNT    0x05    /* set pcb->keep_cnt   - Use number of probes sen
t for get/setsockopt */

Tai-hwa Liang <atliang>
Thu 19 Apr 2007 06:27:42 PM UTC, comment #4: 

Good point... but I still think it would be better not to include the raw-API function definitions in socket.h to prevent users from accessing something directly...

Why is TCP_NODELAY define in tcp.h anyway, when it's only used in sockets.c?

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 19 Apr 2007 06:13:57 PM UTC, comment #3: 

From the CVS log:
revision 1.16
date: 2007-03-05 14:43:11 +0000;  author: fbernon;  state: Exp;  lines: +25 -17
opt.h, sockets.h: add new configuration options (LWIP_POSIX_SOCKETS_IO_NAMES, ETHARP_TRUST_IP_MAC, review SO_REUSE).
Also include directly tcp.h in sockets.h to improve application independancy from ip stack (avoid to include directly in application the "unknown" tcp.h if you need options like TCP_NODELAY and TCP_KEEPALIVE in application.

Jonathan Larmour <jifl>
Project Member
Thu 19 Apr 2007 06:07:40 PM UTC, comment #2: 

Before applying this patch, why is tcp.h included in sockets.h anyway? For me, it compiles without and I think it is better to remove it.

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 12 Apr 2007 09:50:46 AM UTC, comment #1: 

Looks good to me.  I'll apply when I next get the chance

Kieran Mansley <kieranm>
Project MemberIn charge of this item.
Thu 12 Apr 2007 08:50:20 AM UTC, original submission:  

The attached patch tries to add a "lwip/" before local including headers as we already did in the rest of header/implementation files.

Without this patch, users must add include/lwip into their including path.

Tai-hwa Liang <atliang>

 

Attached Files
file #12763:  sockets_h_include.patch added by goldsimon (3KiB - text/x-patch)
file #12471:  socket.h.patch added by atliang (507B - 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 fbernon (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by atliang (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-22 kieranm StatusNone => Done
        Open/ClosedOpen => Closed
    2007-05-13 goldsimon Attached File- => Added sockets_h_include.patch, #12763
    2007-04-12 kieranm Assigned toNone => kieranm
    2007-04-12 atliang Attached File- => Added socket.h.patch, #12471

    Back to the top


    Powered by Savane 3.5