Fix htonl, htons, ntohl, ntohs functions on big endian systems

  The htonl, htons, ntohl and ntohs funtions are defined by POSIX as:

    uint32_t htonl(uint32_t hostlong);
    uint16_t htons(uint16_t hostshort);
    uint32_t ntohl(uint32_t netlong);
    uint16_t ntohs(uint16_t netshort);

They take a uint16_t or uint32_t argument and return a value with the
same type. This means for example that calling htonl on a 64-bit value
should return a 32-bit value.

The GNU libc implements these functions as macros when optimizations are
enabled, and this is explicitely allowed by POSIX. However on big endian
systems there are then defined as:

  # define ntohl(x)      (x)
  # define ntohs(x)      (x)
  # define htonl(x)      (x)
  # define htons(x)      (x)

This means that the values are not casted if the argument is bigger. In
turns that means that:
- the behaviour is different between little and big endian systems;
- the behaviour depends on the optimization level.

This patch attempts to fix that. It adds an implicit cast for ntohl and
htonl using a GCC extension or an explicit one when not using GCC. It
adds an explicit cast for ntohs and htons as the call to __bswap_16
contains an explicit cast.

Changelog:
	* inet/netinet/in.h [__OPTIMIZE__ && (__BYTE_ORDER == __BIG_ENDIAN)]
	(ntohl): Add an implicit uint32_t cast if [__GNUC__ >= 2] or an
	explicit uint32_t cast if [!__GNUC__ >= 2].
	[__OPTIMIZE__ && (__BYTE_ORDER == __BIG_ENDIAN)] (htonl): Likewise.
	[__OPTIMIZE__ && (__BYTE_ORDER == __BIG_ENDIAN)] (ntohs): Add a uint16_t
	cast.
	[__OPTIMIZE__ && (__BYTE_ORDER == __BIG_ENDIAN)] (htons): Likewise.
---
 ChangeLog         | 10 ++++++++++
 inet/netinet/in.h | 19 +++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

Note: the macros in <endian.h> also suffer from the same issue. I'll
send a patch for them if this change is considered acceptable.
  

On Sat, Jul 30, 2016 at 6:59 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> This patch attempts to fix that. It adds an implicit cast for ntohl and
> htonl using a GCC extension or an explicit one when not using GCC. It
> adds an explicit cast for ntohs and htons as the call to __bswap_16
> contains an explicit cast.

I support fixing this.  However, what is the benefit of the "implicit
cast" construct over the "explicit"?  It seems like gratuitous
#ifdeffage at first sight.

zw
  

On 2016-07-30 21:55, Zack Weinberg wrote:
> On Sat, Jul 30, 2016 at 6:59 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> >
> > This patch attempts to fix that. It adds an implicit cast for ntohl and
> > htonl using a GCC extension or an explicit one when not using GCC. It
> > adds an explicit cast for ntohs and htons as the call to __bswap_16
> > contains an explicit cast.
> 
> I support fixing this.  However, what is the benefit of the "implicit
> cast" construct over the "explicit"?  It seems like gratuitous
> #ifdeffage at first sight.

With the implicit cast, GCC still emits a wining when using
-Wconversion, just like using on little endian.

Aurelien
  

* Aurelien Jarno:

> +#  if defined __GNUC__ && __GNUC__ >= 2
> +#   define ntohl(x) \
> +    (__extension__							      \
> +     ({ uint32_t __x = x;						      \

Please use __ntohl_x as the variable name or turn ntohl into a macro
which calls an inline function.  ntohl (__x) looks like something
another header file might do (although the example in gutenprint is
harmless).
  

On Sun, Jul 31, 2016 at 4:59 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On 2016-07-30 21:55, Zack Weinberg wrote:
>> On Sat, Jul 30, 2016 at 6:59 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> >
>> > This patch attempts to fix that. It adds an implicit cast for ntohl and
>> > htonl using a GCC extension or an explicit one when not using GCC. It
>> > adds an explicit cast for ntohs and htons as the call to __bswap_16
>> > contains an explicit cast.
>>
>> I support fixing this.  However, what is the benefit of the "implicit
>> cast" construct over the "explicit"?  It seems like gratuitous
>> #ifdeffage at first sight.
>
> With the implicit cast, GCC still emits a wining when using
> -Wconversion, just like using on little endian.

Oh, I see.  In that case I think I vote for an inline function instead
(this also addresses Florian's concern).

zw
  

On 31 Jul 2016 14:45, Zack Weinberg wrote:
> On Sun, Jul 31, 2016 at 4:59 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On 2016-07-30 21:55, Zack Weinberg wrote:
> >> On Sat, Jul 30, 2016 at 6:59 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> >> >
> >> > This patch attempts to fix that. It adds an implicit cast for ntohl and
> >> > htonl using a GCC extension or an explicit one when not using GCC. It
> >> > adds an explicit cast for ntohs and htons as the call to __bswap_16
> >> > contains an explicit cast.
> >>
> >> I support fixing this.  However, what is the benefit of the "implicit
> >> cast" construct over the "explicit"?  It seems like gratuitous
> >> #ifdeffage at first sight.
> >
> > With the implicit cast, GCC still emits a wining when using
> > -Wconversion, just like using on little endian.
> 
> Oh, I see.  In that case I think I vote for an inline function instead
> (this also addresses Florian's concern).

agreed
-mike