Date: Fri, 10 Jan 2014 16:17:24 +0400 From: "Alexander V. Chernikov" <melifaro@FreeBSD.org> To: Gleb Smirnoff <glebius@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r260488 - head/sys/net Message-ID: <52CFE4D4.8080700@FreeBSD.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 10.01.2014 14:14, Gleb Smirnoff wrote:
> Alexander,
>
> some nitpicking:
>
> 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>
> 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);
> A> +#endif /* SCTP */
> A> +#endif
> 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.
This actually needs to be removed from here and converted to be
ifaddr_event consumer.
>
> A> +
> A> +/*
> A> + * Announce interface address arrival/withdraw
> 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));
> A> +
> A> + if (fibnum != RT_ALL_FIBS) {
> A> + KASSERT(fibnum >= 0 && fibnum < rt_numfibs, ("%s: "
> A> + "fibnum out of range 0 <= %d < %d", __func__,
> A> + fibnum, rt_numfibs));
> A> + }
> A> +
> 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().
Yes, thanks.
>
> A> +
> A> +/*
> A> + * Announce route addition/removal
> 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
Fixed.
>
> A> + * to app (and any other rtsock consumers) with rtm_errno field set to
> A> + * non-zero value.
> A> + * Returns 0 on success.
> A> + */
>
> 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;
> A>
> A> - rt_newaddrmsg_fib(cmd, ifa, error, rt, RT_ALL_FIBS);
> A> + if (route_cb.any_count == 0)
> A> + return (0);
> A> +
> 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> +
> A> + if (fibnum != RT_ALL_FIBS) {
> A> + M_SETFIB(m, fibnum);
> A> + m->m_flags |= RTS_FILTER_FIB;
> A> + }
> A> +
> A> + rt_dispatch(m, sa ? sa->sa_family : AF_UNSPEC);
> A> +
> A> + return (0);
> A> }
> A>
> A> +
>
> Why extra line here?
Fixed.
>
> 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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52CFE4D4.8080700>