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

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

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

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

                 Summary: Sockets API: use OS's sys/socket.h instead of
lwip/sock
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: jllledo
            Submitted on: Sun 21 May 2017 12:10:40 PM CEST
                Category: sockets/netconn
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:

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.




    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Sun 21 May 2017 12:10:40 PM CEST  Name:
0001-Sockets-API-use-OS-s-sys-socket.h-instead-of-lwip-so.patch  Size: 11kB  
By: jllledo

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

    _______________________________________________________

Reply to this item at:

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

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


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

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

madhu
Follow-up Comment #1, patch #9350 (project lwip):

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

    _______________________________________________________

Reply to this item at:

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

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


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

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

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

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

    _______________________________________________________

Reply to this item at:

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

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


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

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

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

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.

    _______________________________________________________

Reply to this item at:

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

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


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

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

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

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

    _______________________________________________________

Reply to this item at:

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

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


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

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

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

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

Additional Item Attachment:

File name: 0001-Sockets-API-use-OS-s-sys-socket.h-instead-of-lwip-so.patch
Size:15 KB


    _______________________________________________________

Reply to this item at:

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

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


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

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

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

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

Additional Item Attachment:

File name: 0001-Allow-the-use-of-external-Sockets-headers.patch Size:64 KB


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9350>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


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

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

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

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?

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9350>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


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

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

madhu
Update of patch #9350 (project lwip):

                  Status:                    None => Done                  
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #8:

Applied, thanks for your contribution!

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9350>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


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

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

madhu
Update of patch #9350 (project lwip):

                  Status:                    Done => None                  
             Open/Closed:                  Closed => Open                  

    _______________________________________________________

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

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9350>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


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

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

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

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

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9350>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


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

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

madhu
Update of patch #9350 (project lwip):

                  Status:                    None => Done                  
             Assigned to:                    None => goldsimon              
             Open/Closed:                    Open => Closed                

    _______________________________________________________

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

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9350>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


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