Date: Fri, 10 Jan 2014 22:21:17 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Gleb Smirnoff <glebius@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, "Alexander V. Chernikov" <melifaro@freebsd.org> Subject: Re: svn commit: r260488 - head/sys/net Message-ID: <20140110215002.F2219@besplex.bde.org> In-Reply-To: <20140110101442.GB73147@FreeBSD.org> References: <201401091813.s09IDPlU058184@svn.freebsd.org> <20140110101442.GB73147@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/proc.h>
> A> #include <sys/domain.h>
> A> #include <sys/kernel.h>
> A> +#include <sys/kdb.h>
Further unsorting of an unsorted list by adding to the end of it.
> A>
> A> #include <net/if.h>
> A> #include <net/if_var.h>
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140110215002.F2219>