buglwIP - A Lightweight TCP/IP stack - Bugs: bug #35928, BSD sockets functions must set...

 
 

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

bug #35928: BSD sockets functions must set errno for POSIX-compliance

Submitter:  Mason <mason>
Submitted:  Wed 21 Mar 2012 09:39:03 AM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Tue 11 Mar 2014 07:54:32 PM UTC, comment #13: 

Fixed (more or less like proposed in the last patch file, but default is to set errno).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 09 Apr 2012 03:34:11 PM UTC, comment #12: 

Sorry for not getting round to this sooner.  It changes the port API and so shouldn't be in 1.4.1.

Let's get it into the devel branch and it will then definitely be in 1.5

I would still like to get rid of the ERRNO define altogether if we can rather than renaming it (and take the opportunity to clean this up) but that requires understanding what it was there for in the first place, and at the moment I don't have time to do that.

Ideally we should consider getting it down to be just:

 #ifndef set_errno
 #define set_errno(err) errno = (err)
 #endif

as in the original report.

As to your point that only the (old, unsupported) coldfire port setting ERRNO: there are many other ports that haven't been officially donated to the project, and so don't feature in the contrib module.  We have to consider those too.

Kieran Mansley <kieranm>
Group Member
Fri 06 Apr 2012 08:19:32 AM UTC, comment #11: 

Can this bug be marked as planned for a specific release? (Either 1.5.0 or 1.4.1) That way, it will appear when searching for open bugs in a given release. Thanks.

Mason <mason>
Wed 28 Mar 2012 09:46:06 AM UTC, comment #10: 

Simon Goldschmidt wrote:

> Mason wrote:
>
>> please consider making LWIP_SOCKET_SET_ERRNO=1 the default.
>
> Wouldn't LWIP_SOCKET_SET_ERRNO=LWIP_COMPAT_SOCKETS be the correct
> default to prevent lwIP's socket implementation messing with the
> global errno?


(Nit: Under POSIX, errno is not global anymore, it is a
thread-local resource)

In my port, I've defined
#define LWIP_COMPAT_SOCKETS   0
#define LWIP_SOCKET_SET_ERRNO 1
and the BSD-socket function renaming is done in cc.h

Maybe I'm using lwip in a very specialized way. (I need the
real POSIX names exported in the library).

My preference is for LWIP_SOCKET_SET_ERRNO to default to
LWIP_SOCKET, but I won't lose sleep if you pick another
default, now that I set LWIP_SOCKET_SET_ERRNO explicitly
in lwipopts.h (others may fall into this trap however).

Mason <mason>
Wed 28 Mar 2012 08:49:13 AM UTC, comment #9: 


> If it doesn't, please consider including it into 1.5,


I will definitively fix this in 1.5.0 (1.4.1 remains to be decided).

> and also please consider making LWIP_SOCKET_SET_ERRNO=1 the default.


Wouldn't LWIP_SOCKET_SET_ERRNO=LWIP_COMPAT_SOCKETS be the correct default to prevent lwIP's socket implementation messing with the global errno?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 Mar 2012 08:25:19 AM UTC, comment #8: 

Simon Goldschmidt wrote:

> I do understand it's not cool lwIP provides 'errno' but doesn't set
> it. However, since this is a potentially port-breaking change, one
> might argue whether 1.4.1 or 1.5.0 is the correct version for this
> change. I'll leave it up to our project leader to decide... (although
> I'd probably vote for 1.4.1 :)


I understand your perspective.

If my proposed change makes it into 1.4.1, I'd be a happy camper.

If it doesn't, please consider including it into 1.5, and also
please consider making LWIP_SOCKET_SET_ERRNO=1 the default.

Mason <mason>
Tue 27 Mar 2012 05:48:30 PM UTC, comment #7: 

I do understand it's not cool lwIP provides 'errno' but doesn't set it. However, since this is a potentially port-breaking change, one might argue whether 1.4.1 or 1.5.0 is the correct version for this change. I'll leave it up to our project leader to decide... (although I'd probably vote for 1.4.1 :)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 26 Mar 2012 11:36:56 AM UTC, comment #6: 

Simon, Kieran,

Could you please comment on the last patch? (where I propose renaming ERRNO to LWIP_SOCKET_SET_ERRNO, and documenting the macro in src/include/lwip/opt.h)

https://savannah.nongnu.org/bugs/download.php?file_id=25430

Can you merge this patch in the 1.4.1 branch?

Mason <mason>
Wed 21 Mar 2012 03:04:34 PM UTC, comment #5: 

Trying to upload the correct patch.

(file #25430)

Mason <mason>
Wed 21 Mar 2012 02:52:44 PM UTC, comment #4: 

Simon Goldschmidt wrote:

> I can understand you're upset about wasting several hours. Still, I
> don't think defining LWIP_SOCKET is the correct guard here: we don't
> define other functions to their standard name by default (e.g.
> 'bind()', 'listen()' etc.) but prefix them with 'lwip_' and only use
> standard names when LWIP_COMPAT_SOCKETS==1.


You do have a point which I'd lost sight of.

As I've mentioned in previous messages, LWIP_COMPAT_SOCKETS
is insufficient to port libcurl, because its configure script
expects the "raw" BSD names in the binary code.

What I have at the moment:


lwipopts.h
#define LWIP_COMPAT_SOCKETS 0

cc.h
#define lwip_gethostbyname    gethostbyname
#define lwip_gethostbyname_r  gethostbyname_r
#define lwip_getaddrinfo      getaddrinfo
#define lwip_freeaddrinfo     freeaddrinfo

#define lwip_accept       accept
#define lwip_bind         bind
#define lwip_connect      connect
#define lwip_getpeername  getpeername
#define lwip_getsockname  getsockname
#define lwip_getsockopt   getsockopt
#define lwip_setsockopt   setsockopt
#define lwip_listen       listen
#define lwip_recv         recv
#define lwip_recvfrom     recvfrom
#define lwip_send         send
#define lwip_sendto       sendto
#define lwip_select       select
#define lwip_shutdown     shutdown
#define lwip_socket       socket

#include <fcntl.h> /*** F_GETFL, F_SETFL, O_NONBLOCK ***/


> Using this, we should be good and backwards compatible:
>
> #ifndef set_errno
> #if LWIP_COMPAT_SOCKETS || defined(ERRNO)
> #define set_errno(err) errno = (err)
> #else /* LWIP_COMPAT_SOCKETS || defined(ERRNO) */
> #define set_errno(err)
> #endif /* LWIP_COMPAT_SOCKETS || defined(ERRNO) */
> #endif /* set_errno */


OK, you guys have made it clear that you don't want to kill off
the stupid ERRNO macro :-)

I propose renaming it, to make it self-explanatory, and documenting
it in opt.h (cf. attached patch)

Do you agree with the latest patch?


(file #25428)

Mason <mason>
Wed 21 Mar 2012 01:21:33 PM UTC, comment #3: 

I can understand you're upset about wasting several hours. Still, I don't think defining LWIP_SOCKET is the correct guard here: we don't define other functions to their standard name by default (e.g. 'bind()', 'listen()' etc.) but prefix them with 'lwip_' and only use standard names when LWIP_COMPAT_SOCKETS==1.

Using this, we should be good and backwards compatible:

#ifndef set_errno
#if LWIP_COMPAT_SOCKETS || defined(ERRNO)
#define set_errno(err) errno = (err)
#else /* LWIP_COMPAT_SOCKETS || defined(ERRNO) */
#define set_errno(err)
#endif /* LWIP_COMPAT_SOCKETS || defined(ERRNO) */
#endif /* set_errno */

Simon Goldschmidt <goldsimon>
Group administrator
Wed 21 Mar 2012 12:37:06 PM UTC, comment #2: 

For the record, the only port that defines ERRNO is coldfire:
(I wonder how the unix port even works!)

$ grep -rn [^_]ERRNO contrib-1.4.0
contrib-1.4.0/ports/old/coldfire/include/arch/errno.h:39:#define ERRNO


Do you at least agree that if one defines LWIP_SOCKET, this means one
wants the BSD sockets compatibility layer, and it is very likely that
one expects the standard behavior from the BSD sockets functions?
(Which is to set errno, sorry if I seem insistent.)

As for why this code was introduced, perhaps you can ask David Haas
and Marc Boucher who wrote it 9 years ago.

http://git.savannah.gnu.org/cgit/lwip.git/commit/src/api/sockets.c?id=dd2fa15e8af974c1a624aa531eb96562878d6414
author davidhaas 2003-02-06 22:18:56 (GMT)

Perhaps it was an experimental feature at the time, and they didn't
feel confident about modifying errno? However, today the stack has
matured, and normal users (like me) expect the sockets compatibility
layer to behave in a standard way. (I wasted several hours debugging
libcurl/lwip code before finding what was wrong.)

In my private tree, I have already applied the proposed patch, but
I don't like maintaining out-of-tree patches.

Mason <mason>
Wed 21 Mar 2012 11:37:27 AM UTC, comment #1: 

I'm cautious about making changes that would require all the existing ports to be changed, which this would need.  We don't understand why this code was introduced, and I'd like to do that before removing it.  For now, please define ERRNO in your port and you will get the behaviour you need.

Kieran Mansley <kieranm>
Group Member
Wed 21 Mar 2012 09:39:03 AM UTC, original submission:  

I am porting libcurl on top of lwip on a (mostly) POSIX-compliant embedded
platform. In this configuration, libcurl expects a POSIX-compliant network
stack. On a closely related note, cf. task #11879: Add POSIX conformant header files.

lwip provides a BSD sockets compatibility layer which, by default, is not
POSIX-compliant, because none of the sockets function set errno.

cf. set_errno and sock_set_errno macros



#ifdef ERRNO
#ifndef set_errno
#define set_errno(err) errno = (err)
#endif
#else /* ERRNO */
#define set_errno(err)
#endif /* ERRNO */

#define sock_set_errno(sk, e) do { \
  sk->err = (e); \
  set_errno(sk->err); \
} while (0)


Thus, by default, set_errno is a NOP.
(NB: the ERRNO macro is undocumented)

Consider the requirements for connect:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/connect.html

  Upon successful completion, connect() shall return 0;
  otherwise, -1 shall be returned and errno set to indicate the error.

(The same requirement exists for all BSD sockets functions.)

In lwip, the (optional) BSD sockets compatibility layer is compiled
only if LWIP_SOCKET is defined to a non-zero value. If an lwip
integrator needs the sockets layer, it is very likely that he expects
the BSD sockets functions to comply to POSIX.

Thus, if LWIP_SOCKET is defined, I believe the BSD sockets functions
must set errno by default. It might make sense to allow integrators
to disable this feature, but compliance should be the default.

Which means applying the following trivial patch, to remove the
undocumented ERRNO macro.



--- sockets.c        2012-02-23 10:57:12.796875000 +0100
+++ sockets.c        2012-03-20 10:50:40.375000000 +0100
@@ -158,13 +158,9 @@
   ((unsigned)(-(err)) < ERR_TO_ERRNO_TABLE_SIZE ? \
     err_to_errno_table[-(err)] : EIO)

-#ifdef ERRNO
 #ifndef set_errno
 #define set_errno(err) errno = (err)
 #endif
-#else /* ERRNO */
-#define set_errno(err)
-#endif /* ERRNO */

 #define sock_set_errno(sk, e) do { \
   sk->err = (e); \


An integrator can
#define set_errno(err)
in his port's cc.h if he needs to disable setting errno (why?!)

Please seriously consider this patch, libcurl checks errno all over
the place.

Mason <mason>

 

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

Attached Files
file #25430:  errno_patch.txt added by mason (1KiB - text/plain)
file #25429:  errno_patch.txt added by mason (1KiB - text/plain - Correct patch using #if not #ifdef)
file #25428:  errno_patch.txt added by mason (1KiB - text/plain)
file #25426:  patch_sockets.txt added by mason (422B - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by mason (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-03-11 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2012-04-09 kieranm Planned Release 1.5.0
    2012-03-21 mason Attached File- Added errno_patch.txt, #25430
    2012-03-21 mason Attached File- Added errno_patch.txt, #25429
    2012-03-21 mason Attached File- Added errno_patch.txt, #25428
    2012-03-21 mason Attached File- Added patch_sockets.txt, #25426

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code