Re: [PATCH 7/9] netconfig: Move loading settings to new method, refactor

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

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

On Wed, 25 Aug 2021 at 15:53, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/23/21 9:14 AM, Andrew Zaborowski wrote:
> > Split loading settings out of network_configure into a new method,
> > network_load_settings.  Make sure both consistently handle errors by
> > printing messages and informing the caller.
> > ---
> >   src/netconfig.c | 157 +++++++++++++++++++++++++++++++-----------------
> >   src/netconfig.h |   5 +-
> >   src/p2p.c       |  12 +++-
> >   src/station.c   |  20 +++---
> >   4 files changed, 127 insertions(+), 67 deletions(-)
> >
>
>    CC       src/netconfig.o
> src/netconfig.c: In function ‘netconfig_get_static4_address’:
> src/netconfig.c:298:5: error: implicit declaration of function
> ‘util_netmask_from_prefix’ [-Werror=implicit-function-declaration]
>    298 |     util_netmask_from_prefix(prefix_len)) {
>        |     ^~~~~~~~~~~~~~~~~~~~~~~~
> src/netconfig.c:297:29: error: comparison of integer expressions of different
> signedness: ‘__uint32_t’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
>    297 |   if (ntohl(in_addr.s_addr) !=
>        |                             ^~
> cc1: all warnings being treated as errors

Oops, apparently I added the #include in the wrong commit.

>
> <snip>
>
> > @@ -998,22 +997,22 @@ static int validate_dns_list(int family, char **dns_list)
> >       return n_valid;
> >   }
> >
> > -bool netconfig_configure(struct netconfig *netconfig,
> > +bool netconfig_load_settings(struct netconfig *netconfig,
> >                               const struct l_settings *active_settings,
> > -                             const uint8_t *mac_address,
> > -                             netconfig_notify_func_t notify, void *user_data)
> > +                             const uint8_t *mac_address)
> >   {
> >       char *mdns;
> > -     char hostname[HOST_NAME_MAX + 1];
> >       bool send_hostname;
> > +     bool enabled;
> >
> > +     l_strfreev(netconfig->dns4_overrides);
>
> ? shouldn't this be taken care of by netconfig_reset?

I thought we might have a future use case for calling
netconfig_load_settings() multiple times without netconfig_reset
between, but no strong motivation for that.

>  We generally don't like
> side-effects in our APIs.

I wouldn't call avoiding a leak a side effect ;)

>
> >       netconfig->dns4_overrides = l_settings_get_string_list(active_settings,
> >                                                       "IPv4", "DNS", ' ');
> >
> >       if (netconfig->dns4_overrides) {
> >               int r = validate_dns_list(AF_INET, netconfig->dns4_overrides);
> >
> > -             if (r <= 0) {
> > +             if (unlikely(r <= 0)) {
> >                       l_strfreev(netconfig->dns4_overrides);
> >                       netconfig->dns4_overrides = NULL;
> >               }
> > @@ -1022,13 +1021,14 @@ bool netconfig_configure(struct netconfig *netconfig,
> >                       l_error("netconfig: Empty IPv4.DNS entry, skipping...");
> >       }
> >
> > +     l_strfreev(netconfig->dns6_overrides);
>
> Same here?
>
> >       netconfig->dns6_overrides = l_settings_get_string_list(active_settings,
> >                                                       "IPv6", "DNS", ' ');
> >
> >       if (netconfig->dns6_overrides) {
> >               int r = validate_dns_list(AF_INET6, netconfig->dns6_overrides);
> >
> > -             if (r <= 0) {
> > +             if (unlikely(r <= 0)) {
> >                       l_strfreev(netconfig->dns6_overrides);
> >                       netconfig->dns6_overrides = NULL;
> >               }
> > @@ -1038,8 +1038,6 @@ bool netconfig_configure(struct netconfig *netconfig,
> >       }
> >
> >       netconfig->active_settings = active_settings;
>
> Do we even need active settings now?

So in this method I only removed the usage of the l_settings from
netconfig_configure() but not from other places (*_get_gateway(),
netconfig_set_domains()) which may be called asynchronously.  It's
something we could do separately if desired but my use case was only
for setting the value of netconfig->rtm_protocol early.

Best regards