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

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

URL:
  <http://savannah.nongnu.org/patch/?5865>

                 Summary: Fix missing lwip directory in socket.h including
path
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: atliang
            Submitted on: 木曜日 2007年04月12日 at 08:50
                Category: None
                Priority: 3 - Low
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any

    _______________________________________________________

Details:

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.



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: 木曜日 2007年04月12日 at 08:50  Name: socket.h.patch  Size: 507B
 By: atliang

<http://savannah.nongnu.org/patch/download.php?file_id=12471>

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Update of patch #5865 (project lwip):

             Assigned to:                    None => kieranm                

    _______________________________________________________

Follow-up Comment #1:

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #2, patch #5865 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #3, patch #5865 (project lwip):

>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.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #4, patch #5865 (project lwip):

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?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #5, patch #5865 (project lwip):

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 */

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #6, patch #5865 (project lwip):

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...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #7, patch #5865 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #8, patch #5865 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #9, patch #5865 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #10, patch #5865 (project lwip):

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.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #11, patch #5865 (project lwip):

>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)
    _______________________________________________________

Additional Item Attachment:

File name: sockets_h_include.patch        Size:3 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #12, patch #5865 (project lwip):

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.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #13, patch #5865 (project lwip):

>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!

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #14, patch #5865 (project lwip):

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 ?


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #15, patch #5865 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #16, patch #5865 (project lwip):

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...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #17, patch #5865 (project lwip):

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.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #18, patch #5865 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

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

Ondrej Lufinka

Follow-up Comment #19, patch #5865 (project lwip):

Kieran, I think you can close it...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5865>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
12