On 17.02.21, 23:41, "Dnsmasq-discuss on behalf of Geert Stappers"
<dnsmasq-discuss-boun...@lists.thekelleys.org.uk on behalf of
stapp...@stappers.nl> wrote:
> > @@ -567,6 +568,12 @@ struct ipsets {
> > struct ipsets *next;
> > };
> >
> > +struct allowlist {
> > + uint32_t mark, mask;
> > + char **patterns;
> > + struct allowlist *next;
> > +};
> > +
>
> I think the missing '# ifdef HAVE_CONNTRACK' will trigger "unused struct"
> warnings ...
> > struct ipsets *ipsets;
> > + uint32_t allowlist_mask;
>
> I think the missing '# ifdef HAVE_CONNTRACK' will trigger "unused uint32_t"
> warnings ...
I have tested compilation with both no-conntrack and HAVE_CONNTRACK on
Raspberry Pi OS, and encountered no compile warnings. Likewise, I have
tested with both HAVE_CONNTRACK and HAVE_UBUS on OpenWrt (for Ubus).
Technically, you are right that this could be guarded as you suggest.
I was thinking about guarding this, but other structs that are only used
optionally, such as the `struct ipsets` directly above this are not
guarded with a similar check.
Besides being consistent with the existing code style, this makes it a
tiny bit less error-prone when having a partial re-compile where some
files use a stale version of the headers with different #ifdefs, which
can introduce very subtle bugs at development time when switching cfg
because the memory layout in the struct would change.
> > +#if defined(HAVE_CONNTRACK) && defined(HAVE_UBUS)
>
> One of many
> > +# ifdef HAVE_CONNTRACK
>
> One of many
Sorry, I don't understand the comment for these. As those features need
libraries that are only present when the corresponding defines are set,
the usage code also needs to be guarded.
Also, as this introduces a new feature I wanted to minimize impact on
any existing installations that do not already use optional features.
Code is only compiled with -DHAVE_CONNTRACK (and -DHAVE_UBUS), and only
activated when the config file enables it (default = disabled).
> .... snip .....
>
> > + if (0);
> > +#ifdef HAVE_CONNTRACK
> > + else if (!allowed)
> > + {
> > + m = setup_reply(header, n, /* addrp: */ NULL, /* flags: */ 0, /*
> > ttl: */ 0);
> > + if (m >= 1)
> > + {
> > + send_from(listen->fd, option_bool(OPT_NOWILD) ||
> > option_bool(OPT_CLEVERBIND),
> > + (char *)header, m, &source_addr, &dst_addr, if_index);
> > + daemon->metrics[METRIC_DNS_LOCAL_ANSWERED]++;
> > + }
> > + }
> > +#endif
> > #ifdef HAVE_AUTH
> > - if (auth_dns)
> > + else if (auth_dns)
>
> That extra else feels odd.
You snipped one line too much at the top, I re-added it here.
If (0);
The previous logic was:
if (auth_dns) ....
The new intended logic is:
if (!allowed) ....
else if (auth_dns) ....
Because the allowed case is in an #ifdef, the logic is like this:
if (0);
#ifdef HAVE_CONNTRACK
else if (!allowed) ....
#endif
else if (auth_dns) ....
In the case where HAVE_CONNTRACK is not defined, this becomes:
if (0);
else if (auth_dns) ....
which is equivalent to:
if (auth_dns) ....
In the case where HAVE_CONNTRACK is defined, this becomes:
if (0);
else if (!allowed) ....
else if (auth_dns) ....
which is equivalent to:
if (!allowed) ....
else if (auth_dns) ....
> > - else
> > #endif
> > + else
>
> That swap of lines feels odd.
This can be explained in a similar way to the one above, it is just some
trickery to stack regular C ifs and pre-processor ifs.
> Do know that it is _not_ up to me to decide on this patch.
>
> Thing I'm saying is that it got some human attention.
Thanks for taking your time to look into it. Appreciate the comments!
> Regards
> Geert Stappers
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
