Re: [PATCH 1/2] avoid using inet_ntoa()

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3067 bytes --]

Hi Davide,

>> That would be unfortunate.  I'm really not sure whether this needs to be 'fixed'
>> anyway.  ell is meant for single-threaded, event driven apps.  So our use of
>> inet_ntoa is just fine.  In fact we do similar stuff all over the place.
> 
> there's nothing wrong with using inet_ntoa() as long as the caller
> doesn't need to be "IPv6 friendly" and concurrency is not an issue.
> However,
> this would result in disabling the whole fedora "badfuncs" CI, that
> now is failing just because of inet_ntoa().
> 

Is this a worthy goal in and of itself though?  What other 'badfuncs' are there?

>>> At this point, probably the cleanest thing to do is to replace
>>> IP_STR() with a proper call to inet_ntop(), preceded by declaration of
>>> buf[INET_ADDRSTRLEN] (like it's already done in some hunks of this
>>> patch). Luckily, it's just 10 places (versus 6 users of inet_ntoa(),
>>> still looks feasible).
>>
>> Perhaps we can do something similar to MAC_STR instead?
> 
> that's  similar to the NIPQUAD / NIPQUAD_FMT thing that was done in
> Linux before the %pI4 format specified was introduced in printk (
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3685f25de1b0447fff381c420de
> ) :
> 
> #include <stdio.h>
> #include <netinet/in.h>
> #include <arpa/inet.h>
> 
> #define IP "%u.%u.%u.%u"
> #define IP_STR(uint_ip) ((unsigned char *) &uint_ip)[0], \
>                          ((unsigned char *) &uint_ip)[1], \
>                          ((unsigned char *) &uint_ip)[2], \
>                          ((unsigned char *) &uint_ip)[3]
> 
> int main(int argc, const char *argv[])
> {
>          struct in_addr ia = { .s_addr = 0x010200c0 };
>          char buf[INET_ADDRSTRLEN];
> 
>          printf("test 0 %s\n", inet_ntoa(ia));
>          printf("test 1 %s\n", inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN));
>          printf("test 2 " IP "\n", IP_STR(ia.s_addr));
> 
>          return 0;
> }
> 

I think this or something like this would be quite reasonable to use in / for 
any debug messages where we print an ip.

> 
>>> Il giorno lun 7 giu 2021 alle ore 20:41 Denis Kenzior
>>> <denkenz@gmail.com> ha scritto:
>>>>
>>>>> that might be useful for other future CLIENT_DEBUG() users.
>>>>>
>>>>
>>>> Fair enough,  Wish there was a printf extension for ipv4 and ipv6 addresses like
>>>> printk uses...  Oh well.
>>>
>>> yes, that would really make this game less boring :)
>>>
>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
>>> [2] https://github.com/shemminger/iproute2/blob/main/lib/utils.c#L980-L1020
>>>
>>
>> Yep, that's the other thing we can do.  Just re-implement inet_ntoa or make our
>> own version that covers v4/v6 with a static buffer.  Not sure that is any better
>> than just using inet_ntoa though ;)
> 
> what about converting inet_ntoa() into inet_ntop(), and re-writing
> IP_STR() as above?

I guess that would be OK.  We just have to remember to not use inet_ntoa in the 
future...

Regards,
-Denis