From owner-svn-src-all@freebsd.org Fri Jan 27 20:28:05 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 99C0BCC4127; Fri, 27 Jan 2017 20:28:05 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (glebi.us [96.95.210.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 84658CF4; Fri, 27 Jan 2017 20:28:04 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.15.2/8.15.2) with ESMTPS id v0RKS3a1039132 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 27 Jan 2017 12:28:04 -0800 (PST) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebi.us (8.15.2/8.15.2/Submit) id v0RKS3AB039131; Fri, 27 Jan 2017 12:28:03 -0800 (PST) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@FreeBSD.org using -f Date: Fri, 27 Jan 2017 12:28:03 -0800 From: Gleb Smirnoff To: Bruce Evans Cc: Konstantin Belousov , Luiz Otavio O Souza , 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: <20170127202803.GU2611@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> <20170127171655.V2822@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170127171655.V2822@besplex.bde.org> User-Agent: Mutt/1.7.2 (2016-11-26) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Jan 2017 20:28:05 -0000 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.