Re: [PATCH 07/14] netconfig: Add getters for DNS addresses and domain names

[Andrew Zaborowski <andrew.zaborowski@intel.com>]

Hi Denis,

On Fri, 17 Jun 2022 at 18:32, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> On 6/15/22 17:47, Andrew Zaborowski wrote:
> > ---
> >   ell/ell.sym     |  2 ++
> >   ell/netconfig.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   ell/netconfig.h |  2 ++
> >   3 files changed, 68 insertions(+)
> >
>
> So I applied all patches in this series

Great.

> except this one.  Just a minor nit here:
>
> > diff --git a/ell/ell.sym b/ell/ell.sym
> > index 50f8909..d74e8c9 100644
> > --- a/ell/ell.sym
> > +++ b/ell/ell.sym
> > @@ -757,6 +757,8 @@ global:
> >       l_netconfig_apply_rtnl;
> >       l_netconfig_get_addresses;
> >       l_netconfig_get_routes;
> > +     l_netconfig_get_dns_list;
> > +     l_netconfig_get_domain_names;
> >   local:
> >       *;
> >   };
> > diff --git a/ell/netconfig.c b/ell/netconfig.c
> > index ce5bfe3..94faef0 100644
> > --- a/ell/netconfig.c
> > +++ b/ell/netconfig.c
> > @@ -1425,3 +1425,67 @@ LIB_EXPORT const struct l_queue_entry *l_netconfig_get_routes(
> >
> >       return l_queue_get_entries(netconfig->routes.current);
> >   }
> > +
> > +/* Returns a new strv array to be freed by the caller */
> > +LIB_EXPORT char **l_netconfig_get_dns_list(struct l_netconfig *netconfig)
> > +{
> > +     char **ret = NULL;
> > +     const struct l_dhcp_lease *v4_lease;
> > +     const struct l_dhcp6_lease *v6_lease;
> > +
> > +#define CONCAT(dest, src, free)              \
>
> Why do we define this in the function body itself?  That seems weird for a macro
> that is used outside of this function.

True, I could have placed it before the function start.

>
> > +     do {                            \
> > +             char **tmp = src;       \
> > +             tmp = free ? tmp : l_strv_copy(tmp);    \
> > +             if (!dest)              \
> > +                     dest = tmp;     \
> > +             else if (tmp) {         \
> > +                     unsigned int destlen = l_strv_length(dest);     \
> > +                     unsigned int srclen = l_strv_length(tmp);       \
> > +                     dest = l_realloc(dest, sizeof(char *) * \
> > +                             (destlen + srclen + 1));        \
> > +                     memcpy(dest + destlen, tmp, sizeof(char *) *    \
> > +                                     (srclen + 1));  \
> > +                     l_free(tmp);    \
> > +             }                       \
> > +     } while (0)
>
> But really, can't we just turn this macro into a static convenience function
> instead?  Let the compiler decide whether inlining is appropriate.

Ok.

Best regards