From owner-svn-src-head@FreeBSD.ORG Fri Jan 10 11:21:28 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id C16BD4B7; Fri, 10 Jan 2014 11:21:28 +0000 (UTC) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 6A27B122B; Fri, 10 Jan 2014 11:21:28 +0000 (UTC) Received: from c122-106-144-87.carlnfd1.nsw.optusnet.com.au (c122-106-144-87.carlnfd1.nsw.optusnet.com.au [122.106.144.87]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 7A633780678; Fri, 10 Jan 2014 22:21:18 +1100 (EST) Date: Fri, 10 Jan 2014 22:21:17 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Gleb Smirnoff Subject: Re: svn commit: r260488 - head/sys/net In-Reply-To: <20140110101442.GB73147@FreeBSD.org> Message-ID: <20140110215002.F2219@besplex.bde.org> References: <201401091813.s09IDPlU058184@svn.freebsd.org> <20140110101442.GB73147@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.1 cv=HZAtEE08 c=1 sm=1 tr=0 a=p/w0leo876FR0WNmYI1KeA==:117 a=PO7r1zJSAAAA:8 a=eMIcmV_oiccA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=N3VrZbfzcpQA:10 a=mmTdQZTImtcw-5-rgbgA:9 a=LxpqiRVzYvIhZ244:21 a=SfjsROqbNFa2OhEC:21 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, "Alexander V. Chernikov" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Jan 2014 11:21:28 -0000 On Fri, 10 Jan 2014, Gleb Smirnoff wrote: > Alexander, > > some nitpicking: Some more: > > On Thu, Jan 09, 2014 at 06:13:25PM +0000, Alexander V. Chernikov wrote: > A> @@ -52,6 +53,7 @@ > A> #include > A> #include > A> #include > A> +#include Further unsorting of an unsorted list by adding to the end of it. > A> > A> #include > A> #include > A> @@ -86,6 +88,13 @@ > A> #define RT_NUMFIBS 1 > A> #endif > A> > A> +#if defined(INET) || defined(INET6) > A> +#ifdef SCTP > A> +extern void sctp_addr_change(struct ifaddr *ifa, int cmd); Redundant extern. This was redundant even for 1978 K&R, though it may have been needed for some non-C compilers, and for styles that want to emphasize that things are extern. KNF is not such a style. Parameter names in prototypes. style(9) and actual style are very inconsistent about this. The nearby style should be copied. This file (route,c) is old so it probably doesn't use parameter names in prototypes. It used to forward-declare a couple of static functions, and it didn't use parameter names in them. These functions went away. Now it still has some static functions, but doesn't forward-declare any of them (another style bug). > A> +#endif /* SCTP */ Excessive commenting of endifs. style(9) has very detailed rules on comments on endifs. > A> +#endif Inconsistent excessive commenting of endifs (not excessive here). > A> + > A> + > > Can be simplified to one liner: > > #if (defined(INET) || defined(INET6)) && defined(SCTP) > > Same stands for same ifdef down below in code. > > And extra empty line shouldn't have been added. > > A> + > A> +/* > A> + * Announce interface address arrival/withdraw Missing punctuation. > A> + * Returns 0 on success. > A> + */ > A> +int > A> +rt_addrmsg(int cmd, struct ifaddr *ifa, int fibnum) > A> +{ > A> + > A> + KASSERT(cmd == RTM_ADD || cmd == RTM_DELETE, > A> + ("unexpected cmd %u", cmd)); Non-KNF continuation indentation (1 tab instead of 4 spaces). Type mismatch (unsigned format, int arg). > A> + Extra blank line. > A> + if (fibnum != RT_ALL_FIBS) { > A> + KASSERT(fibnum >= 0 && fibnum < rt_numfibs, ("%s: " Obfuscation of comment by splitting it in the middle. Doesn't even reduce the amount of line-splittling. > A> + "fibnum out of range 0 <= %d < %d", __func__, KNF continuation indentation now (not a style bug). Obfuscation of function name using __func__. > A> + fibnum, rt_numfibs)); Non-KNF continuation indentation (5 spaces instead of 4). > A> + } > A> + Extra blank line. > A> + return (rtsock_addrmsg(cmd, ifa, fibnum)); > A> +} > > Second KASSERT together with if clause can be simplified to: > > KASSERT(fibnum == RT_ALL_FIBS || (fibnum >= 0 && fibnum < rt_numfibs), ... > > Same simplification can be done in rt_routemsg() and rt_newaddrmsg_fib(). > > A> + > A> +/* > A> + * Announce route addition/removal Missing punctuation. > A> + * Users of this function MUST validate input data BEFORE calling. > A> + * However we have to be able to handle invalid data: > A> + * if some userland app sends us "invalid" route message (invalid mask, > A> + * no dst, wrokg address families, etc...) we need to pass it back > ^ > typo > > A> + * to app (and any other rtsock consumers) with rtm_errno field set to > A> + * non-zero value. > A> + * Returns 0 on success. > A> + */ Multi-line comments don't look like the above. See style(9) for how they look. > > A> +int > A> +rtsock_routemsg(int cmd, struct ifnet *ifp, int error, struct rtentry *rt, > A> + int fibnum) > A> { > A> + struct rt_addrinfo info; > A> + struct sockaddr *sa; > A> + struct mbuf *m; > A> + struct rt_msghdr *rtm; Minor disorder. > A> > A> - rt_newaddrmsg_fib(cmd, ifa, error, rt, RT_ALL_FIBS); > A> + if (route_cb.any_count == 0) > A> + return (0); > A> + Extra blank line. > A> + bzero((caddr_t)&info, sizeof(info)); > A> + info.rti_info[RTAX_NETMASK] = rt_mask(rt); > A> + info.rti_info[RTAX_DST] = sa = rt_key(rt); > A> + info.rti_info[RTAX_GATEWAY] = rt->rt_gateway; > A> + if ((m = rt_msg1(cmd, &info)) == NULL) > A> + return (ENOBUFS); > A> + rtm = mtod(m, struct rt_msghdr *); > A> + rtm->rtm_index = ifp->if_index; > A> + rtm->rtm_flags |= rt->rt_flags; > A> + rtm->rtm_errno = error; > A> + rtm->rtm_addrs = info.rti_addrs; > A> + Extra blank line. > A> + if (fibnum != RT_ALL_FIBS) { > A> + M_SETFIB(m, fibnum); > A> + m->m_flags |= RTS_FILTER_FIB; > A> + } > A> + Extra blank line. > A> + rt_dispatch(m, sa ? sa->sa_family : AF_UNSPEC); > A> + Extra blank line. > A> + return (0); > A> } > A> > A> + > > Why extra line here? Sticky return key ;). indent -sob will remove all the extra blank lines in the above, but will also remove some that shouldn't be removed. I like to use blank lines within functions only before blocks preceded by a comment. indent -sob removes even these. > > A> /* > A> * This is the analogue to the rt_newaddrmsg which performs the same > A> * function but for multicast group memberhips. This is easier since Example in old code of how block comments look. They are "filled so they look like real paragraphs", with sentence breaks of 2 spaces. Bruce