From owner-svn-src-all@freebsd.org Wed Mar 15 01:53:30 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D8554D0ACF3; Wed, 15 Mar 2017 01:53:30 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 7BB64135E; Wed, 15 Mar 2017 01:53:30 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 6E720105F1F; Wed, 15 Mar 2017 12:53:21 +1100 (AEDT) Date: Wed, 15 Mar 2017 12:53:20 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eric van Gyzen 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 In-Reply-To: <201703141827.v2EIRmLv080307@repo.freebsd.org> Message-ID: <20170315110727.M964@besplex.bde.org> References: <201703141827.v2EIRmLv080307@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=KeqiiUQD c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=wRcB3c5BtoXeLLscNvUA:9 a=Pq97Be7vu5kELnKR:21 a=ugwPLQF33PDyiZ65:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Mar 2017 01:53:31 -0000 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