Date: Wed, 15 Mar 2017 12:53:20 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Eric van Gyzen <vangyzen@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r315277 - in head/sys: dev/cxgb/ulp/iw_cxgb netinet Message-ID: <20170315110727.M964@besplex.bde.org> In-Reply-To: <201703141827.v2EIRmLv080307@repo.freebsd.org> References: <201703141827.v2EIRmLv080307@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 14 Mar 2017, Eric van Gyzen wrote:
> Log:
> KTR: log IPv4 addresses in hex rather than dotted-quad
>
> When I made the changes in r313821, I fell victim to one of the
> classic blunders, the most famous of which is: never get involved
> in a land war in Asia. But only slightly less well known is this:
> Keep your brain turned on and engaged when making a tedious, sweeping,
> mechanical change. KTR can correctly log the immediate integral values
> passed to it, as well as constant strings, but not non-constant strings,
> since they might change by the time ktrdump retrieves them.
>
> Reported by: glebius
> MFC after: 3 days
> Sponsored by: Dell EMC
The new format is too raw. Hex is just as easy to read as dotted-quad,
but inet_ntoa() also does the equivalent of htonl(). Due to not using
it, addresses are now printed reversed in most but not all cases on
little-endian arches. They should be converted to host byte order
using ntohl() except when they are already in host byte order.
Old code already prints some addresses using the bad format %lx or worse
(this should be 0x%08x like new code) and still does this conversion.
This conversion is still correct, except it doesn't do the reversal on
little-endian arches, so users would have to guess which cases are
reversed.
Addresses are sometimes in host byte order to begin with. Old code
might print these in hex without conversion, but when it called
inet_ntoa() it first converted to network byte order using htonl().
This now gives consistent reversals on little-endian arches.
This commit also fixes the style bug of messes from using the
badly-designed API inet_ntoa_r(), but not nearby style bugs.
> Modified: head/sys/dev/cxgb/ulp/iw_cxgb/iw_cxgb_cm.c
> ==============================================================================
> --- head/sys/dev/cxgb/ulp/iw_cxgb/iw_cxgb_cm.c Tue Mar 14 18:08:32 2017 (r315276)
> +++ head/sys/dev/cxgb/ulp/iw_cxgb/iw_cxgb_cm.c Tue Mar 14 18:27:48 2017 (r315277)
> @@ -1481,9 +1478,8 @@ process_data(struct iwch_ep *ep)
> */
> in_getsockaddr(ep->com.so, (struct sockaddr **)&local);
> in_getpeeraddr(ep->com.so, (struct sockaddr **)&remote);
> - CTR3(KTR_IW_CXGB, "%s local %s remote %s", __FUNCTION__,
> - inet_ntoa_r(local->sin_addr, local_str),
> - inet_ntoa_r(remote->sin_addr, remote_str));
> + CTR3(KTR_IW_CXGB, "%s local 0x%08x remote 0x%08x", __FUNCTION__,
> + local->sin_addr.s_addr, remote->sin_addr.s_addr);
> ep->com.local_addr = *local;
> ep->com.remote_addr = *remote;
> free(local, M_SONAME);
Still has 3 style bugs:
- continuation indent of 8 instead of KNF 4
- obfuscates the function name by not using a string literal
- doesn't even spell the obfuscation using its C99 name __func__.
CTR*() uses obfuscated macros which prevent checking for printf() format
errors. The macros convert all args to u_long, so all formats except %lx
are technically broken. Using htonl() would also convert to a consistent
type that is technically correct. htonl() used to return long, but it
now returns uint32_t, and the type hacks for CTR*() depend on much the
same magic as long sort of being the same as uint32_t for the purposes
of htonl(). But changing the type of htonl() broke lots of printf()
formats, expecially in code that was careful enough to use the right
format to print longs.
[... another instances of the above style bugs, and more __FUNCTION__'s
visible in unchanged code.
> Modified: head/sys/netinet/igmp.c
> ==============================================================================
> --- head/sys/netinet/igmp.c Tue Mar 14 18:08:32 2017 (r315276)
> +++ head/sys/netinet/igmp.c Tue Mar 14 18:27:48 2017 (r315277)
> @@ -312,17 +312,6 @@ igmp_scrub_context(struct mbuf *m)
> m->m_pkthdr.flowid = 0;
> }
>
> -#ifdef KTR
> -static __inline char *
> -inet_ntoa_haddr(in_addr_t haddr, char *addrbuf)
> -{
> - struct in_addr ia;
> -
> - ia.s_addr = htonl(haddr);
> - return (inet_ntoa_r(ia, addrbuf));
> -}
> -#endif
This did a double reversal on little-endian arches. The first reversal
is now done directly. So the order is even more confusing but less
MD than I first thought. I think the address now ends up looking
consistently backwards on little-endian arches (provided all callers
remember to do the above conversion iff their address starts in host
byte order).
> @@ -875,9 +861,9 @@ igmp_input_v2_query(struct ifnet *ifp, c
> */
> inm = inm_lookup(ifp, igmp->igmp_group);
> if (inm != NULL) {
> - CTR3(KTR_IGMPV3, "process v2 query %s on ifp %p(%s)",
> - inet_ntoa_r(igmp->igmp_group, addrbuf), ifp,
> - ifp->if_xname);
> + CTR3(KTR_IGMPV3,
> + "process v2 query 0x%08x on ifp %p(%s)",
> + igmp->igmp_group.s_addr, ifp, ifp->if_xname);
> igmp_v2_update_group(inm, timer);
> }
> }
This doesn't have the obfuscations to print the function's name, since it
doesn't print the name.
Like for KASSERT(), the function name probably shouldn't supplied explicitly
or omitted explicity fby the caller, but should be supplied automatically
like for assert(3), but not as badly implemented as for assert(3). It
can be recovered from the program counter in a way that shouldn't pessimize
for space or time. For KTR, it seems to be necessary to pessimize in the
generated record (if the user wants function names). The program counter
would have to be recorded. Always recording it would be shorter than
usually recording pointers to the function's name or literal strings.
> @@ -907,12 +893,9 @@ out_locked:
> static void
> igmp_v2_update_group(struct in_multi *inm, const int timer)
> {
> -#ifdef KTR
> - char addrbuf[INET_ADDRSTRLEN];
> -#endif
>
> - CTR4(KTR_IGMPV3, "%s: %s/%s timer=%d", __func__,
> - inet_ntoa_r(inm->inm_addr, addrbuf), inm->inm_ifp->if_xname, timer);
> + CTR4(KTR_IGMPV3, "0x%08x: %s/%s timer=%d", __func__,
> + inm->inm_addr.s_addr, inm->inm_ifp->if_xname, timer);
This uses the usual obfuscation of the function's name.
> ...
Most calls print the function name using the __func__ obfuscation, and
don't have indentation errors, and are prefectly backwards for htonl()/
ntohl().
> ...
> Modified: head/sys/netinet/ip_mroute.c
> ==============================================================================
> --- head/sys/netinet/ip_mroute.c Tue Mar 14 18:08:32 2017 (r315276)
> +++ head/sys/netinet/ip_mroute.c Tue Mar 14 18:27:48 2017 (r315277)
> @@ -1066,8 +1060,8 @@ add_mfc(struct mfcctl2 *mfccp)
>
> /* If an entry already exists, just update the fields */
> if (rt) {
> - CTR4(KTR_IPMF, "%s: update mfc orig %s group %lx parent %x",
> - __func__, inet_ntoa_r(mfccp->mfcc_origin, addrbuf),
> + CTR4(KTR_IPMF, "%s: update mfc orig 0x%08x group %lx parent %x",
> + __func__, mfccp->mfcc_origin.s_addr,
> (u_long)ntohl(mfccp->mfcc_mcastgrp.s_addr),
> mfccp->mfcc_parent);
> update_mfc_params(rt, mfccp);
This one already printed 1 address in confusing hex format with a not-so
bogus cast and a confusing host/network conversion for the printing, and
and another confusing hex format.
The old hex formats are missing 0x prefixes, so are more confusing that
before now that there is a hex format not missing the prefix on the same
line.
ntohl() no longer returns long, but is cast to u_long for portability.
This might be best for printf(), but CTR*() does the same cast. The
'l' in %lx is also technically needed and it is not having 'l' for %x
in all CTR*() is technically wrong,
(ktrdump copies the args to an array of u_long's and then applies
the format string to this, and %x's instead of %lx's in it usually
work magically)
but it is best to consistently omit casts and consistently use wrong %x
formats.
Finally, the ntohl() is confusing. I think it is actually correct,
and is what this commit should have added where inet_ntoa() was used
with no htonl() before it. Apparently the variable is in network byte
order, and we need to fudge it into host byte order because printf()
will convert from host byte order to network byte order. printf()
actually converts host integers in host byte order to big-endian order
for printing, but since network byte order is big endian this is like
htonl() to reverse the ntohl() in the above.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170315110727.M964>