Re: svn commit: r312770 - in head/sys: net netinet netinet6
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-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r312770 - in head/sys: net netinet netinet6
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
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r312770 - in head/sys: net netinet netinet6
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.
--
Totus tuus, Glebius.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r312770 - in head/sys: net netinet netinet6
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
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r312770 - in head/sys: net netinet netinet6
On Thu, Jan 26, 2017 at 12:26:32AM +0200, Konstantin Belousov wrote:
K> On Wed, Jan 25, 2017 at 02:20:06PM -0800, Gleb Smirnoff wrote:
K> > Thanks, Luiz!
K> >
K> > One stylistic nit that I missed in review:
K> >
K> > L> static int
K> > L> -in_difaddr_ioctl(caddr_t data, struct ifnet *ifp, struct thread *td)
K> > L> +in_difaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, struct
thread *td)
K> > L> {
K> > L> const struct ifreq *ifr = (struct ifreq *)data;
K> > L> const struct sockaddr_in *addr = (const struct sockaddr_in *)
K> > L> @@ -618,7 +618,8 @@ in_difaddr_ioctl(caddr_t data, struct if
K> > L> in_ifadown(&ia->ia_ifa, 1);
K> > L>
K> > L> if (ia->ia_ifa.ifa_carp)
K> > L> - (*carp_detach_p)(&ia->ia_ifa);
K> > L> + (*carp_detach_p)(&ia->ia_ifa,
K> > L> + (cmd == SIOCDIFADDR) ? false : true);
K> >
K> > Can we change the very last line to:
K> >
K> >(cmd == SIOCAIFADDR) ? true : false);
K> This is just 'cmd == SIOCAIFADDR'.
Right. Would be even better :)
--
Totus tuus, Glebius.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r312770 - in head/sys: net netinet netinet6
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);
This is just 'cmd == SIOCAIFADDR'.
>
> I think that would be more straightforward. Do you agree? Sorry for not
> noticing that before.
>
> --
> Totus tuus, Glebius.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r312770 - in head/sys: net netinet netinet6
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);
I think that would be more straightforward. Do you agree? Sorry for not
noticing that before.
--
Totus tuus, Glebius.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r312770 - in head/sys: net netinet netinet6
Author: loos
Date: Wed Jan 25 19:04:08 2017
New Revision: 312770
URL: https://svnweb.freebsd.org/changeset/base/312770
Log:
After the in_control() changes in r257692, an existing address is
(intentionally) deleted first and then completely added again (so all the
events, announces and hooks are given a chance to run).
This cause an issue with CARP where the existing CARP data structure is
removed together with the last address for a given VHID, which will cause
a subsequent fail when the address is later re-added.
This change fixes this issue by adding a new flag to keep the CARP data
structure when an address is not being removed.
There was an additional issue with IPv6 CARP addresses, where the CARP data
structure would never be removed after a change and lead to VHIDs which
cannot be destroyed.
Reviewed by: glebius
Obtained from:pfSense
MFC after:2 weeks
Sponsored by: Rubicon Communications, LLC (Netgate)
Modified:
head/sys/net/if.c
head/sys/netinet/in.c
head/sys/netinet/ip_carp.c
head/sys/netinet/ip_carp.h
head/sys/netinet6/in6.c
Modified: head/sys/net/if.c
==
--- head/sys/net/if.c Wed Jan 25 18:31:51 2017(r312769)
+++ head/sys/net/if.c Wed Jan 25 19:04:08 2017(r312770)
@@ -145,7 +145,7 @@ int (*carp_output_p)(struct ifnet *ifp,
const struct sockaddr *sa);
int(*carp_ioctl_p)(struct ifreq *, u_long, struct thread *);
int(*carp_attach_p)(struct ifaddr *, int);
-void (*carp_detach_p)(struct ifaddr *);
+void (*carp_detach_p)(struct ifaddr *, bool);
#endif
#ifdef INET
int(*carp_iamatch_p)(struct ifaddr *, uint8_t **);
Modified: head/sys/netinet/in.c
==
--- head/sys/netinet/in.c Wed Jan 25 18:31:51 2017(r312769)
+++ head/sys/netinet/in.c Wed Jan 25 19:04:08 2017(r312770)
@@ -71,7 +71,7 @@ __FBSDID("$FreeBSD$");
#include
static int in_aifaddr_ioctl(u_long, caddr_t, struct ifnet *, struct thread *);
-static int in_difaddr_ioctl(caddr_t, struct ifnet *, struct thread *);
+static int in_difaddr_ioctl(u_long, caddr_t, struct ifnet *, struct thread *);
static voidin_socktrim(struct sockaddr_in *);
static voidin_purgemaddrs(struct ifnet *);
@@ -245,7 +245,7 @@ in_control(struct socket *so, u_long cmd
break;
case SIOCDIFADDR:
sx_xlock(&in_control_sx);
- error = in_difaddr_ioctl(data, ifp, td);
+ error = in_difaddr_ioctl(cmd, data, ifp, td);
sx_xunlock(&in_control_sx);
return (error);
case OSIOCAIFADDR: /* 9.x compat */
@@ -390,7 +390,7 @@ in_aifaddr_ioctl(u_long cmd, caddr_t dat
IF_ADDR_RUNLOCK(ifp);
if (ia != NULL)
- (void )in_difaddr_ioctl(data, ifp, td);
+ (void )in_difaddr_ioctl(cmd, data, ifp, td);
ifa = ifa_alloc(sizeof(struct in_ifaddr), M_WAITOK);
ia = (struct in_ifaddr *)ifa;
@@ -528,7 +528,7 @@ fail2:
fail1:
if (ia->ia_ifa.ifa_carp)
- (*carp_detach_p)(&ia->ia_ifa);
+ (*carp_detach_p)(&ia->ia_ifa, false);
IF_ADDR_WLOCK(ifp);
TAILQ_REMOVE(&ifp->if_addrhead, &ia->ia_ifa, ifa_link);
@@ -545,7 +545,7 @@ fail1:
}
static int
-in_difaddr_ioctl(caddr_t data, struct ifnet *ifp, struct thread *td)
+in_difaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, struct thread
*td)
{
const struct ifreq *ifr = (struct ifreq *)data;
const struct sockaddr_in *addr = (const struct sockaddr_in *)
@@ -618,7 +618,8 @@ in_difaddr_ioctl(caddr_t data, struct if
in_ifadown(&ia->ia_ifa, 1);
if (ia->ia_ifa.ifa_carp)
- (*carp_detach_p)(&ia->ia_ifa);
+ (*carp_detach_p)(&ia->ia_ifa,
+ (cmd == SIOCDIFADDR) ? false : true);
/*
* If this is the last IPv4 address configured on this
Modified: head/sys/netinet/ip_carp.c
==
--- head/sys/netinet/ip_carp.c Wed Jan 25 18:31:51 2017(r312769)
+++ head/sys/netinet/ip_carp.c Wed Jan 25 19:04:08 2017(r312770)
@@ -1969,7 +1969,7 @@ carp_attach(struct ifaddr *ifa, int vhid
}
void
-carp_detach(struct ifaddr *ifa)
+carp_detach(struct ifaddr *ifa, bool keep_cif)
{
struct ifnet *ifp = ifa->ifa_ifp;
struct carp_if *cif = ifp->if_carp;
@@ -2015,12 +2015,13 @@ carp_detach(struct ifaddr *ifa)
carp_hmac_prepare(sc);
carp_sc_state(sc);
- if (sc->sc_naddrs == 0 && sc->sc_naddrs6 == 0)
+ if (!keep_cif && sc->sc_naddrs == 0 && sc->sc_naddrs6 == 0)
carp_destroy(sc);
else
CARP_UNLOCK(sc);
- CIF_FREE(cif);
+ if (!keep_cif)
+ CIF_FREE(cif);
sx_xunlock(&carp_
