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