Date: Fri, 27 Jan 2017 17:34:45 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Gleb Smirnoff <glebius@freebsd.org> Cc: Bruce Evans <brde@optusnet.com.au>, Konstantin Belousov <kostikbel@gmail.com>, Luiz Otavio O Souza <loos@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312770 - in head/sys: net netinet netinet6 Message-ID: <20170127171655.V2822@besplex.bde.org> In-Reply-To: <20170126215927.GL2611@FreeBSD.org> References: <201701251904.v0PJ48YF061428@repo.freebsd.org> <20170125222006.GH2611@FreeBSD.org> <20170125222632.GQ2349@kib.kiev.ua> <20170126133341.V1087@besplex.bde.org> <20170126215927.GL2611@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 26 Jan 2017, Gleb Smirnoff wrote:
> On Thu, Jan 26, 2017 at 02:03:05PM +1100, Bruce Evans wrote:
> B> On Thu, 26 Jan 2017, Konstantin Belousov wrote:
> B>
> B> > On Wed, Jan 25, 2017 at 02:20:06PM -0800, Gleb Smirnoff wrote:
> B> >> Thanks, Luiz!
> B> >>
> B> >> One stylistic nit that I missed in review:
> B> >>
> B> >> L> static int
> B> >> L> -in_difaddr_ioctl(caddr_t data, struct ifnet *ifp, struct thread *td)
> B> >> L> +in_difaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, struct thread *td)
> B> >> L> {
> B> >> L> const struct ifreq *ifr = (struct ifreq *)data;
> B> >> L> const struct sockaddr_in *addr = (const struct sockaddr_in *)
> B> >> L> @@ -618,7 +618,8 @@ in_difaddr_ioctl(caddr_t data, struct if
> B> >> L> in_ifadown(&ia->ia_ifa, 1);
> B> >> L>
> B> >> L> if (ia->ia_ifa.ifa_carp)
> B> >> L> - (*carp_detach_p)(&ia->ia_ifa);
> B> >> L> + (*carp_detach_p)(&ia->ia_ifa,
> B> >> L> + (cmd == SIOCDIFADDR) ? false : true);
> B> >>
> B> >> Can we change the very last line to:
> B> >>
> B> >> (cmd == SIOCAIFADDR) ? true : false);
> B>
> B> That is not stylistic, but invert the result. Perhaps you meant to
> B> reverse the test to avoid negative logic for the result.
>
> It uses different ioctl value, so it doesn't invert result. Instead
> of !SIOCDIFADDR I want more explicit SIOCAIFADDR.
Oops. So it is non-stylistic in a different way. cmd can only be
SIOCDIFADDR, or one or both of SIOCAIFADDR. Than is unclear. Assuming
that the original code is correct and that all 3 cases can occur,
inversion would break all 3 cases, while the non-stylistic change breaks
only the O_SIOCAIFADDR case.
Since there can be more than 2 cases and it isn't clear that there are
at most 3, any boolean test on 1 of the cases is going to be unclear.
Positive logic will be clearer, but that requires comparison with 2
cases. The current code use negative logic to select these 2 cases as
the complement of the other case.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170127171655.V2822>