patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9350, Sockets API: use OS's sys/socket.h...

 
 

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

patch #9350: Sockets API: use OS's sys/socket.h instead of lwip/sock

Submitted by:  Joan Lledó <jllledo>
Submitted on:  Sun 21 May 2017 10:10:40 AM UTC  
 
Category: sockets/netconnPriority: 5 - Normal
Status: NonePrivacy: Public
Assigned to: NoneOpen/Closed: Open
Planned Release: None

Tue 23 May 2017 07:12:38 AM UTC, comment #5:

> Do you really think? I thought we could just move all defines and
> typedefs to one region (which is left out in your case) and have
> the function definitions always included and it should work?


Yeah, it should work, what I meant is it doesn't allow the user to provide its own sys/socket.h w/o changes in LwIP.

> And if you don't make it without changes, we should at least try to
> only require changing API files, not core files like ip.h


Well, that's an easier target. I wrote a new patch that creates a new header and doesn't include it if LWIP_SYS_SOCKET is defined. The new header only includes the items in the POSIX standard, so it works and pass the tests if the user doesn't define LWIP_SYS_SOCKET, but the user has to remove manually the other conflicting macros and structs if s/he defines it.

> We'd have to fix the odd implicit dependency from ip.h to sockets.h of course.


I don't know how to do this. In the patch, the solution is a bit tricky: if the user doesn't define LWIP_SYS_SOCKET, then the new header takes the values from ip.h, otherwise, the dependency is inverted and ip.h is getting the values from the OS's sys/socket.h. Any better idea?

(file #40766)

Joan Lledó <jllledo>
Mon 22 May 2017 07:35:13 PM UTC, comment #4:

> I don't think that will work


Do you really think? I thought we could just move all defines and typedefs to one region (which is left out in your case) and have the function definitions always included and it should work?
We'd have to fix the odd implicit dependency from ip.h to sockets.h of course.

> probably we should forget it


I'd really love to support you here. Although the BSD license allows it, I hate to have too many forks: they tend to make more and more changes to the original code and one day, they're so far away that merging patches becomes a pain. I really want to help people trying to push back changes, even if they are as funny as yours.

And if you don't make it without changes, we should at least try to only require changing API files, not core files like ip.h ;-)

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 22 May 2017 07:27:33 PM UTC, comment #3:

Hi Simon,

> Maybe we can move all the "sys/socket.h" definitions to a
> seperate file and completely prevent including this
> lwip-sys-socket.h if you have your own?


If the target is to provide the user with a way to include its own sys/sockets.h without changing anything in LwIP, I don't think that will work, because we still have the same problem: to not knowing what to move to the new header and what not.

As you said, there isn't an easy solution to this and probably we should forget it.

Joan Lledó <jllledo>
Mon 22 May 2017 06:51:32 PM UTC, comment #2:

I'm sorry to disappoint you, but your patch looks exactly how I imagined, it seems to be just fitted to your needs to get it to compile.
This isn't meant offensive, I said before that I don't know where to draw the line of what to override and what not (and how to do it).
While the SYS_SOCKET part is only a bit ugly (but I'd be OK with that), all the other ifdefs are rather horrible. I know we already have some of them, but it just looks strange.

Maybe we can move all the "sys/socket.h" definitions to a seperate file and completely prevent including this lwip-sys-socket.h if you have your own?

Oh, and I'm fully aware of the problem in ip.h, I only haven't found a better solution yet. However, using sys/socket.h defines in ip.h is completely breaking layers! Includes should be the other way round (sockets including ip, not ip including sockets).

Simon Goldschmidt <goldsimon>
Project Administrator
Sun 21 May 2017 12:20:55 PM UTC, comment #1:

Ops! I forgot the links!

[1] http://lists.nongnu.org/archive/html/lwip-devel/2017-05/msg00130.html
[2] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_socket.h.html

Joan Lledó <jllledo>
Sun 21 May 2017 10:10:40 AM UTC, original submission:

Hello,

This comes from[1].

This submission includes the patch but, after working a bit more on the issue, I think the patch shouldn't be committed. Following are the problems I found:

1- The patch only covers the macros and structures listed in [2], but those are only a little subset of the macros that are actually used in the real world. For instance, PF_INET is not listed. This means all the macros in sys/socket.h but not in the list have to be wrapped with #ifndef - #endif, and we don't have a clear line to separate what should be wrapped and what not, so the only solution is to wrap everything.

2- An OS's sockets implementation is not limited to sys/sockets.h, there are more headers like netinet/in.h that are not considered in the patch. The patch as it's submitted here works for the Hurd, but fails on Linux. There are many compiler errors because many types in sys/types.h doesn't match the types lwip uses internally, so to make this work, we should change a lot of variable definitions to be something like "typeof(other_variable) var;".

3- Even doing the effort of making points 1 and 2 work, unit tests show that applying the patch breaks some functions like lwip_connect(). Probably because some macro comparison still doesn't match the proper value.

Summarizing, in [1] I thought supporting sys/sockets.h would be enough but it's not, so the patch is incomplete and the gain is not worth the work of research and writing the changes.

Regards.

Joan Lledó <jllledo>

 

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by jllledo (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 23 May 2017 07:12:38 AM UTCjllledoAttached File-=>Added 0001-Sockets-API-use-OS-s-sys-socket.h-instead-of-lwip-so.patch, #40766
    Sun 21 May 2017 10:10:40 AM UTCjllledoAttached File-=>Added 0001-Sockets-API-use-OS-s-sys-socket.h-instead-of-lwip-so.patch, #40750

    Back to the top


    Powered by Savane 3.1-cleanup1