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).
|
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.
|
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.
|
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).
|
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?
|
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.
|
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 :)
|
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?
|
Wed 21 Mar 2012 03:04:34 PM UTC, comment #5:
Trying to upload the correct patch.
(file #25430)
|
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:
> 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)
|
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 */
|
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!)
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.
|
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.
|
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
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.
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.
|