On Fri, Jan 27, 2017 at 05:34:45PM +1100, Bruce Evans wrote:
B> > On Thu, Jan 26, 2017 at 02:03:05PM +1100, Bruce Evans wrote:
B> > B> On Thu, 26 Jan 2017, Konstantin Belousov wrote:
B> > B>
B> > B> > On Wed, Jan 25, 2017 at 02:20:06PM -0800, Gleb Smirnoff wrote:
B> > B> >> Thanks, Luiz!
B> > B> >>
B> > B> >> One stylistic nit that I missed in review:
B> > B> >>
B> > B> >> L> static int
B> > B> >> L> -in_difaddr_ioctl(caddr_t data, struct ifnet *ifp, struct thread
*td)
B> > B> >> L> +in_difaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp,
struct thread *td)
B> > B> >> L> {
B> > B> >> L> const struct ifreq *ifr = (struct ifreq *)data;
B> > B> >> L> const struct sockaddr_in *addr = (const struct sockaddr_in *)
B> > B> >> L> @@ -618,7 +618,8 @@ in_difaddr_ioctl(caddr_t data, struct if
B> > B> >> L> in_ifadown(&ia->ia_ifa, 1);
B> > B> >> L>
B> > B> >> L> if (ia->ia_ifa.ifa_carp)
B> > B> >> L> - (*carp_detach_p)(&ia->ia_ifa);
B> > B> >> L> + (*carp_detach_p)(&ia->ia_ifa,
B> > B> >> L> + (cmd == SIOCDIFADDR) ? false : true);
B> > B> >>
B> > B> >> Can we change the very last line to:
B> > B> >>
B> > B> >> (cmd == SIOCAIFADDR) ? true : false);
B> > B>
B> > B> That is not stylistic, but invert the result. Perhaps you meant to
B> > B> reverse the test to avoid negative logic for the result.
B> >
B> > It uses different ioctl value, so it doesn't invert result. Instead
B> > of !SIOCDIFADDR I want more explicit SIOCAIFADDR.
B>
B> Oops. So it is non-stylistic in a different way. cmd can only be
B> SIOCDIFADDR, or one or both of SIOCAIFADDR. Than is unclear. Assuming
B> that the original code is correct and that all 3 cases can occur,
B> inversion would break all 3 cases, while the non-stylistic change breaks
B> only the O_SIOCAIFADDR case.
B>
B> Since there can be more than 2 cases and it isn't clear that there are
B> at most 3, any boolean test on 1 of the cases is going to be unclear.
B> Positive logic will be clearer, but that requires comparison with 2
B> cases. The current code use negative logic to select these 2 cases as
B> the complement of the other case.
O_SIOCAIFADDR should just be deleted. My suggestion is not only convert
to positive logic, but also outline that SIOCAIFADDR is an exceptional
case, in all other cases in_difaddr_ioctl() which is named "delete ifaddr"
should do delete everything.
--
Totus tuus, Glebius.
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
