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

Submitter:  Joan Lledó <jllledo>
Submitted:  Sun 21 May 2017 10:10:40 AM UTC
   
 
Category:  sockets/netconn Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Fri 26 Oct 2018 05:34:59 PM UTC, comment #11: 

Pushed a different version which doesn't break history. Also I've changed the logic: default of the new option is 0, set it to 1 to include your OS headers.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 22 Oct 2018 06:39:35 PM UTC, comment #10: 

I think we need an "opt-in" define to include the OS headers and we should leave the rest of the 2 header files as they were (no wrapper headers).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 22 Oct 2018 06:38:17 PM UTC, comment #9: 

Reverted:
It breaks the build if LWIP_SOCKET is disabled.
Plus it breaks git history for inet.h and sockets.h for no real reason.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 19 Oct 2018 06:32:30 PM UTC, comment #8: 

Applied, thanks for your contribution!

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 12 Sep 2018 07:43:58 PM UTC, comment #7: 

I guess I'll need a while to catch up on this, the release has currently my top prio with lwip (and lwip doesn't get as much time as it should...)

> On the other hand, I removed the SO_* != SOF_* #errors in init.c b[..]


I think Dirk has already removed them?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 07 Sep 2018 07:53:47 AM UTC, comment #6: 

Hello,

Let's try to close this. I wrote a patch which allows the user to provide their own sockets headers. It includes three new macros LWIP_SOCKET_HEADERS, LWIP_INCLUDE_SOCKETS and LWIP_INCLUDE_INET.

When including their own sockets, the user must probably make some changes in sockets.c, but each particular case will be different, so I don't see the way to do this upstream and therefore excluded sockets.c from the patch.

On the other hand, I removed the SO_* != SOF_* #errors in init.c b/c, as I mentioned in the mailing list, it seems to me they're not needed anymore after commit b034451 and the user won't be able to provide their own headers while these #errors are there



(file #44951)

Joan Lledó <jllledo>
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>
Group 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>
Group administrator
Sun 21 May 2017 12:20:55 PM UTC, comment #1: 
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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-26 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2018-10-22 goldsimon StatusDone None
        Open/ClosedClosed Open
    2018-10-19 dziegel StatusNone Done
        Open/ClosedOpen Closed
    2018-09-07 jllledo Attached File- Added 0001-Allow-the-use-of-external-Sockets-headers.patch, #44951
    2017-05-23 jllledo Attached File- Added 0001-Sockets-API-use-OS-s-sys-socket.h-instead-of-lwip-so.patch, #40766
    2017-05-21 jllledo Attached 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.13-758e.
    Corresponding source code