[Dnsmasq-discuss] [PATCH] Log upstream port for dnssec-retry

[Geert Stappers]

On Sun, Apr 17, 2022 at 12:51:42PM +0200, Dominik Derigs wrote:
> On Fri, 2022-04-15 at 10:17 +0200, Geert Stappers wrote:
> >  2022-04-10, Dominik Derigs wrote:
} } } Subject: [PATCH] Also log upstream port for dnssec-retry
} }  there should be more text about the why.
> > > -               log_query_mysockaddr(F_NOEXTRA | F_DNSSEC, daemon->namebuff, &srv->addr,
> > > -                                    "dnssec-retry", (forward-flags & FREC_DNSKEY_QUERY) ? T_DNSKEY : T_DS);
> > > +               log_query_mysockaddr(F_NOEXTRA | F_DNSSEC | F_SERVER, daemon->namebuff, &srv->addr,
> > > +                                    (forward->flags & FREC_DNSKEY_QUERY) ? "dnssec-retry[DNSKEY]" : "dnssec-retry[DS]", 0);
 
those lines shuffled:
> > > -               log_query_mysockaddr(F_NOEXTRA | F_DNSSEC, daemon->namebuff, &srv->addr,
> > > +               log_query_mysockaddr(F_NOEXTRA | F_DNSSEC | F_SERVER, daemon->namebuff, &srv->addr,
> > > +                                    (forward->flags & FREC_DNSKEY_QUERY) ? "dnssec-retry[DNSKEY]" : "dnssec-retry[DS]", 0);
> > > -                                    "dnssec-retry", (forward-flags & FREC_DNSKEY_QUERY) ? T_DNSKEY : T_DS);

white space change on the above last two lines to align the ,
> > > +     (forward->flags & FREC_DNSKEY_QUERY) ? "dnssec-retry[DNSKEY]" : "dnssec-retry[DS]", 0);
> > > -                                                                         "dnssec-retry", (forward-flags & FREC_DNSKEY_QUERY) ? T_DNSKEY : T_DS);
> > I see more changes as commit message says.
> 
> What do you see in addition?

I was expecting to see only an addition of logging a port number.
The extra '| F_SERVER' did surprise me.
And the '(forward->flags & FREC_DNSKEY_QUERY) ? "dnssec-retry[DNSKEY]" : "dnssec-retry[DS]"'
becoming '"dnssec-retry"', plus a '0' becoming '(forward-flags & FREC_DNSKEY_QUERY) ? T_DNSKEY : T_DS)'
realy made me (poorly) expressing "the patch needs a much better commit message"


> It is a minimal invasive change that fixes the omission in a
> previous commit as already said in the first mail:
> 
> On Sun, 2022-04-10 at 10:46 +0200, Dominik Derigs wrote:
> > This is added by this patch implementing it in the same way as
> > used already when logging "dnssec-query" in the code.
> 
> This is the commit, if you want to compare the change yourself:
> https://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=ff43d35aeef6178f7471c6f37e91845c9a72bd2f

Partial https://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commitdiff;h=ff43d35aeef6178f7471c6f37e91845c9a72bd2f
--- a/src/forward.c
+++ b/src/forward.c
@@ -123,9 +123,17 @@ static void set_outgoing_mark(struct frec *forward, int fd)
 static void log_query_mysockaddr(unsigned int flags, char *name, union mysockaddr *addr, char *arg, unsigned short type)
 {
   if (addr->sa.sa_family == AF_INET)
-    log_query(flags | F_IPV4, name, (union all_addr *)&addr->in.sin_addr, arg, type);
+    {
+      if (flags & F_SERVER)
+       type = ntohs(addr->in.sin_port);
+      log_query(flags | F_IPV4, name, (union all_addr *)&addr->in.sin_addr, arg, type);
+    }
   else
-    log_query(flags | F_IPV6, name, (union all_addr *)&addr->in6.sin6_addr, arg, type);
+    {
+      if (flags & F_SERVER)
+       type = ntohs(addr->in6.sin6_port);
+      log_query(flags | F_IPV6, name, (union all_addr *)&addr->in6.sin6_addr, arg, type);
+    }
 }
 

(saying "link was visited, an attempt to compare was done")

 
> Happy Easter and best regards,
> Dominik
> 

Thanks, you also.

Thing to consider: resubmit the patch with a better commit message.


Other thing we should think about:
A clear signal like "patch is rejected"


Groeten
Geert Stappers
In another attempt to get beyond ignored patches
-- 
Silence is hard to parse