Date: Thu, 26 Jan 2017 14:03:05 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Gleb Smirnoff <glebius@freebsd.org>, 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: <20170126133341.V1087@besplex.bde.org> In-Reply-To: <20170125222632.GQ2349@kib.kiev.ua> References: <201701251904.v0PJ48YF061428@repo.freebsd.org> <20170125222006.GH2611@FreeBSD.org> <20170125222632.GQ2349@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 26 Jan 2017, Konstantin Belousov wrote:
> On Wed, Jan 25, 2017 at 02:20:06PM -0800, Gleb Smirnoff wrote:
>> Thanks, Luiz!
>>
>> One stylistic nit that I missed in review:
>>
>> L> static int
>> L> -in_difaddr_ioctl(caddr_t data, struct ifnet *ifp, struct thread *td)
>> L> +in_difaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, struct thread *td)
>> L> {
>> L> const struct ifreq *ifr = (struct ifreq *)data;
>> L> const struct sockaddr_in *addr = (const struct sockaddr_in *)
>> L> @@ -618,7 +618,8 @@ in_difaddr_ioctl(caddr_t data, struct if
>> L> in_ifadown(&ia->ia_ifa, 1);
>> L>
>> L> if (ia->ia_ifa.ifa_carp)
>> L> - (*carp_detach_p)(&ia->ia_ifa);
>> L> + (*carp_detach_p)(&ia->ia_ifa,
>> L> + (cmd == SIOCDIFADDR) ? false : true);
>>
>> Can we change the very last line to:
>>
>> (cmd == SIOCAIFADDR) ? true : false);
That is not stylistic, but invert the result. Perhaps you meant to
reverse the test to avoid negative logic for the result.
> This is just 'cmd == SIOCAIFADDR'.
Not quite. The result of a relational operator is 1 or 0 (and thus has
type int), not true or false (and thus would have type bool).
Unfortunately, carp has a bool infestation (the declaration of
carp_attach_p() is 1 of 21 lines in netinet with bool), so int is the
wrong type. It is automatically converted to bool by the prototype.
12 of the 21 lines with bools are actually mispelling of 'boolean'
for IP options in in.h. 'bool' is a C type. IP options are never
bools. Some of them are booleans represented as ints (never as bool
since get/setsockopt() only uses int). A meta-comment in in.h
describes this "bool is stored as int".
Fixing the style bugs on 1 line gives another style bug -- line splitting
which was to make space for the verbose spelling of a logical value.
The inversion could also be written less verbosely and more clearly using
the ! operator
!(cmd == SIOCAIFADDR)
but since the expression is so simple the ! operator can be distributed in
it almost as clearly, giving !=. Then remove the parentheses.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170126133341.V1087>