Re: buildkernel failure in net/toeplitz.c (commit 3b281d1421a78b588c5fc4182009ce62d8823d95)

On 2025-02-24 09:54, Mark Johnston wrote:
> On Mon, Feb 24, 2025 at 03:52:14PM +0800, Zhenlei Huang wrote:
>>
>>
>>> On Feb 24, 2025, at 12:42 PM, Ian FREISLICH <ianfreislich@gmail.com> wrote:
>>>
>>> Hi
>>>
>>> Building a kernel today failed with:
>>>
>>> -Werror /usr/src/sys/net/toeplitz.c
>>> In file included from /usr/src/sys/net/toeplitz.c:29:
>>> In file included from /usr/src/sys/net/rss_config.h:33:
>>> /usr/src/sys/netinet/in.h:692:23: error: call to undeclared function 'htonl'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
>>>   692 |         return (in.s_addr == htonl(INADDR_BROADCAST) ||
>>>       |                              ^
>>> In file included from /usr/src/sys/net/toeplitz.c:32:
>>> In file included from /usr/src/sys/sys/systm.h:98:
>>> /usr/src/sys/sys/param.h:343:13: error: conflicting types for 'htonl'
>>>   343 | __uint32_t       htonl(__uint32_t);
>>>       |                  ^
>>> /usr/src/sys/netinet/in.h:692:23: note: previous implicit declaration is here
>>>   692 |         return (in.s_addr == htonl(INADDR_BROADCAST) ||
>>>
>>> I think this is a result of changes to netinet/in.h (3b281d1421a78) which added a static inline function using ntohl() which is not defined in kernel use.
>>>
>>> Ian
>>>
>>
>>
>> May you please have a try with this patch ?
>>
>> ```
>> diff --git a/sys/netinet/in.h b/sys/netinet/in.h
>> index 0925e3aa7669..4dad4e4fed4d 100644
>> --- a/sys/netinet/in.h
>> +++ b/sys/netinet/in.h
>> @@ -689,8 +689,8 @@ void         in_ifdetach(struct ifnet *);
>>   static inline bool
>>   in_broadcast(struct in_addr in)
>>   {
>> -       return (in.s_addr == htonl(INADDR_BROADCAST) ||
>> -           in.s_addr == htonl(INADDR_ANY));
>> +       return (in.s_addr == INADDR_BROADCAST ||
>> +           in.s_addr == INADDR_ANY);
>>   }
>> ```
>>
>> The `htonl` is pointless here, as INADDR_BROADCAST is all 1s, and INADDR_ANY is all 0s.
> 
> See the review: it's formally unneeded, as you say, but I think the
> conversions should be there in case this function is ever augmented
> someday.  I think this patch would also work:
> 
> diff --git a/sys/netinet/in.h b/sys/netinet/in.h
> index fa710af7cd58..e42422341ada 100644
> --- a/sys/netinet/in.h
> +++ b/sys/netinet/in.h
> @@ -689,8 +689,8 @@ void	 in_ifdetach(struct ifnet *);
>   static inline bool
>   in_broadcast(struct in_addr in)
>   {
> -	return (in.s_addr == htonl(INADDR_BROADCAST) ||
> -	    in.s_addr == htonl(INADDR_ANY));
> +	return (in.s_addr == __htonl(INADDR_BROADCAST) ||
> +	    in.s_addr == __htonl(INADDR_ANY));
>   }
>   
>   #define	in_hosteq(s, t)	((s).s_addr == (t).s_addr)

I was going to start a bikeshed: I prefer the style of the original 
because it shows that the author understands the importance of byte 
order. Perhaps the root issue is that htonl is define in both 
arpa/inet.h and netinet/in.h with conditional compilation in and out of 
the kernel. The reason existing uses of htonl work in this header file 
is that they're #defines and not functions and so they're not compiled 
unless they're used.

Ian