hello Denis, thanks for reviewing!

Il giorno ven 4 giu 2021 alle ore 18:01 Denis Kenzior <denkenz@gmail.com> ha scritto:
Hi Davide,
 
[...]
 
Strictly speaking the examples/ change should be in its own patch.  See HACKING.

I should have read it, as well as the coding style rules, before submitting _ sorry for not doing that.
I will address coding style and remove SOB line in the v2.

[...]

> @@ -911,9 +923,10 @@ static void dhcp_client_rx_message(const void *data, size_t len, void *userdata)
>               ia.s_addr = client->lease->address;
>   
>               /* For unit testing we don't want this to be a fatal error */
> -             if (!l_acd_start(client->acd, inet_ntoa(ia))) {
> +             if (!inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) ||
> +                 !l_acd_start(client->acd, buf)) {
>                       CLIENT_DEBUG("Failed to start ACD on %s, continuing",
> -                                             inet_ntoa(ia));
> +                                  IP_STR(ia.s_addr));

Not sure introducing IP_STR macro just for this is worth it, but ok...

that might be useful for other future CLIENT_DEBUG() users.

[...]
 
Looks like indentation problem here too.  Maybe you want to put the inet_ntop
business outside of l_strdup_printf call to make things more readable?

yes, it seems to look better - I will also fix this.
By the way, looking at the inet_ntop() documentation [1], I think that I can also avoid things like

   inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) ? : "(inv)";

because the first argument is hardcoded to AF_INET and also we are sure that 'buf' is INET_ADDRSTRLEN long.
Under these conditions, the return value of inet_ntop() should always be 'buf'. Correct?

--
davide

[1] https://man7.org/linux/man-pages/man3/inet_ntop.3.html
RispondiInoltra