Re: carp_ourether() tweak
On Tue, Feb 06, 2018 at 01:05:44PM +0100, Martin Pieuchot wrote:
> > You're right. The current logic is broken since the last refactoring.
> > It should read "if it's not my MAC nor the one of my carp(4) children".
> > Diff below corrects that in a way that should prevent future refactoring
> > to break it again.
>
> Anyone?
OK bluhm@
> > Index: net/if_bridge.c
> > ===
> > RCS file: /cvs/src/sys/net/if_bridge.c,v
> > retrieving revision 1.301
> > diff -u -p -r1.301 if_bridge.c
> > --- net/if_bridge.c 10 Jan 2018 23:50:39 - 1.301
> > +++ net/if_bridge.c 25 Jan 2018 14:27:43 -
> > @@ -997,6 +997,25 @@ bridgeintr_frame(struct bridge_softc *sc
> > }
> >
> > /*
> > + * Return 1 if `ena' belongs to `ifl', 0 otherwise.
> > + */
> > +int
> > +bridge_ourether(struct bridge_iflist *ifl, uint8_t *ena)
> > +{
> > + struct arpcom *ac = (struct arpcom *)ifl->ifp;
> > +
> > + if (bcmp(ac->ac_enaddr, ena, ETHER_ADDR_LEN) == 0)
> > + return (1);
> > +
> > +#if NCARP > 0
> > + if (carp_ourether(ifl->ifp, ena))
> > + return (1);
> > +#endif
> > +
> > + return (0);
> > +}
> > +
> > +/*
> > * Receive input from an interface. Queue the packet for bridging if its
> > * not for us, and schedule an interrupt.
> > */
> > @@ -1022,7 +1041,6 @@ bridge_process(struct ifnet *ifp, struct
> > struct bridge_iflist *ifl;
> > struct bridge_iflist *srcifl;
> > struct ether_header *eh;
> > - struct arpcom *ac;
> > struct mbuf *mc;
> >
> > ifl = (struct bridge_iflist *)ifp->if_bridgeport;
> > @@ -1105,13 +1123,7 @@ bridge_process(struct ifnet *ifp, struct
> > TAILQ_FOREACH(ifl, >sc_iflist, next) {
> > if (ifl->ifp->if_type != IFT_ETHER)
> > continue;
> > - ac = (struct arpcom *)ifl->ifp;
> > - if (bcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN) == 0
> > -#if NCARP > 0
> > - || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
> > - !carp_ourether(ifl->ifp, eh->ether_dhost))
> > -#endif
> > - ) {
> > + if (bridge_ourether(ifl, eh->ether_dhost)) {
> > if (srcifl->bif_flags & IFBIF_LEARNING)
> > bridge_rtupdate(sc,
> > (struct ether_addr *)>ether_shost,
> > @@ -1129,12 +1141,7 @@ bridge_process(struct ifnet *ifp, struct
> > bridge_ifinput(ifl->ifp, m);
> > return;
> > }
> > - if (bcmp(ac->ac_enaddr, eh->ether_shost, ETHER_ADDR_LEN) == 0
> > -#if NCARP > 0
> > - || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
> > - !carp_ourether(ifl->ifp, eh->ether_shost))
> > -#endif
> > - ) {
> > + if (bridge_ourether(ifl, eh->ether_shost)) {
> > m_freem(m);
> > return;
> > }
> > Index: netinet/ip_carp.c
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> > retrieving revision 1.327
> > diff -u -p -r1.327 ip_carp.c
> > --- netinet/ip_carp.c 12 Jan 2018 23:47:24 - 1.327
> > +++ netinet/ip_carp.c 25 Jan 2018 14:18:44 -
> > @@ -1339,12 +1348,15 @@ carp_iamatch(struct ifnet *ifp)
> > int
> > carp_ourether(struct ifnet *ifp, u_int8_t *ena)
> > {
> > - struct srpl *cif;
> > + struct srpl *cif = >if_carp;
> > struct carp_softc *vh;
> >
> > KERNEL_ASSERT_LOCKED(); /* touching if_carp + carp_vhosts */
> > +
> > + if (SRPL_EMPTY_LOCKED(cif))
> > + return (0);
> > +
> > KASSERT(ifp->if_type == IFT_ETHER);
> > - cif = >if_carp;
> >
> > SRPL_FOREACH_LOCKED(vh, cif, sc_list) {
> > struct carp_vhost_entry *vhe;
> >
Re: carp_ourether() tweak
On 25/01/18(Thu) 15:29, Martin Pieuchot wrote:
> On 24/01/18(Wed) 09:30, Alexander Bluhm wrote:
> > On Mon, Jan 22, 2018 at 11:58:30AM +0100, Martin Pieuchot wrote:
> > > Check if `if_carp' is empty inside carp_ourether() instead of outside.
> > >
> > > ok?
> >
> > Maybe I am confused by the ! and && but I think this diff changes the
> > logic.
>
> You're right. The current logic is broken since the last refactoring.
>
> It should read "if it's not my MAC nor the one of my carp(4) children".
>
> Diff below corrects that in a way that should prevent future refactoring
> to break it again.
Anyone?
> Index: net/if_bridge.c
> ===
> RCS file: /cvs/src/sys/net/if_bridge.c,v
> retrieving revision 1.301
> diff -u -p -r1.301 if_bridge.c
> --- net/if_bridge.c 10 Jan 2018 23:50:39 - 1.301
> +++ net/if_bridge.c 25 Jan 2018 14:27:43 -
> @@ -997,6 +997,25 @@ bridgeintr_frame(struct bridge_softc *sc
> }
>
> /*
> + * Return 1 if `ena' belongs to `ifl', 0 otherwise.
> + */
> +int
> +bridge_ourether(struct bridge_iflist *ifl, uint8_t *ena)
> +{
> + struct arpcom *ac = (struct arpcom *)ifl->ifp;
> +
> + if (bcmp(ac->ac_enaddr, ena, ETHER_ADDR_LEN) == 0)
> + return (1);
> +
> +#if NCARP > 0
> + if (carp_ourether(ifl->ifp, ena))
> + return (1);
> +#endif
> +
> + return (0);
> +}
> +
> +/*
> * Receive input from an interface. Queue the packet for bridging if its
> * not for us, and schedule an interrupt.
> */
> @@ -1022,7 +1041,6 @@ bridge_process(struct ifnet *ifp, struct
> struct bridge_iflist *ifl;
> struct bridge_iflist *srcifl;
> struct ether_header *eh;
> - struct arpcom *ac;
> struct mbuf *mc;
>
> ifl = (struct bridge_iflist *)ifp->if_bridgeport;
> @@ -1105,13 +1123,7 @@ bridge_process(struct ifnet *ifp, struct
> TAILQ_FOREACH(ifl, >sc_iflist, next) {
> if (ifl->ifp->if_type != IFT_ETHER)
> continue;
> - ac = (struct arpcom *)ifl->ifp;
> - if (bcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN) == 0
> -#if NCARP > 0
> - || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
> - !carp_ourether(ifl->ifp, eh->ether_dhost))
> -#endif
> - ) {
> + if (bridge_ourether(ifl, eh->ether_dhost)) {
> if (srcifl->bif_flags & IFBIF_LEARNING)
> bridge_rtupdate(sc,
> (struct ether_addr *)>ether_shost,
> @@ -1129,12 +1141,7 @@ bridge_process(struct ifnet *ifp, struct
> bridge_ifinput(ifl->ifp, m);
> return;
> }
> - if (bcmp(ac->ac_enaddr, eh->ether_shost, ETHER_ADDR_LEN) == 0
> -#if NCARP > 0
> - || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
> - !carp_ourether(ifl->ifp, eh->ether_shost))
> -#endif
> - ) {
> + if (bridge_ourether(ifl, eh->ether_shost)) {
> m_freem(m);
> return;
> }
> Index: netinet/ip_carp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.327
> diff -u -p -r1.327 ip_carp.c
> --- netinet/ip_carp.c 12 Jan 2018 23:47:24 - 1.327
> +++ netinet/ip_carp.c 25 Jan 2018 14:18:44 -
> @@ -1339,12 +1348,15 @@ carp_iamatch(struct ifnet *ifp)
> int
> carp_ourether(struct ifnet *ifp, u_int8_t *ena)
> {
> - struct srpl *cif;
> + struct srpl *cif = >if_carp;
> struct carp_softc *vh;
>
> KERNEL_ASSERT_LOCKED(); /* touching if_carp + carp_vhosts */
> +
> + if (SRPL_EMPTY_LOCKED(cif))
> + return (0);
> +
> KASSERT(ifp->if_type == IFT_ETHER);
> - cif = >if_carp;
>
> SRPL_FOREACH_LOCKED(vh, cif, sc_list) {
> struct carp_vhost_entry *vhe;
>
Re: carp_ourether() tweak
On 24/01/18(Wed) 09:30, Alexander Bluhm wrote:
> On Mon, Jan 22, 2018 at 11:58:30AM +0100, Martin Pieuchot wrote:
> > Check if `if_carp' is empty inside carp_ourether() instead of outside.
> >
> > ok?
>
> Maybe I am confused by the ! and && but I think this diff changes the
> logic.
You're right. The current logic is broken since the last refactoring.
It should read "if it's not my MAC nor the one of my carp(4) children".
Diff below corrects that in a way that should prevent future refactoring
to break it again.
ok?
Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.301
diff -u -p -r1.301 if_bridge.c
--- net/if_bridge.c 10 Jan 2018 23:50:39 - 1.301
+++ net/if_bridge.c 25 Jan 2018 14:27:43 -
@@ -997,6 +997,25 @@ bridgeintr_frame(struct bridge_softc *sc
}
/*
+ * Return 1 if `ena' belongs to `ifl', 0 otherwise.
+ */
+int
+bridge_ourether(struct bridge_iflist *ifl, uint8_t *ena)
+{
+ struct arpcom *ac = (struct arpcom *)ifl->ifp;
+
+ if (bcmp(ac->ac_enaddr, ena, ETHER_ADDR_LEN) == 0)
+ return (1);
+
+#if NCARP > 0
+ if (carp_ourether(ifl->ifp, ena))
+ return (1);
+#endif
+
+ return (0);
+}
+
+/*
* Receive input from an interface. Queue the packet for bridging if its
* not for us, and schedule an interrupt.
*/
@@ -1022,7 +1041,6 @@ bridge_process(struct ifnet *ifp, struct
struct bridge_iflist *ifl;
struct bridge_iflist *srcifl;
struct ether_header *eh;
- struct arpcom *ac;
struct mbuf *mc;
ifl = (struct bridge_iflist *)ifp->if_bridgeport;
@@ -1105,13 +1123,7 @@ bridge_process(struct ifnet *ifp, struct
TAILQ_FOREACH(ifl, >sc_iflist, next) {
if (ifl->ifp->if_type != IFT_ETHER)
continue;
- ac = (struct arpcom *)ifl->ifp;
- if (bcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN) == 0
-#if NCARP > 0
- || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
- !carp_ourether(ifl->ifp, eh->ether_dhost))
-#endif
- ) {
+ if (bridge_ourether(ifl, eh->ether_dhost)) {
if (srcifl->bif_flags & IFBIF_LEARNING)
bridge_rtupdate(sc,
(struct ether_addr *)>ether_shost,
@@ -1129,12 +1141,7 @@ bridge_process(struct ifnet *ifp, struct
bridge_ifinput(ifl->ifp, m);
return;
}
- if (bcmp(ac->ac_enaddr, eh->ether_shost, ETHER_ADDR_LEN) == 0
-#if NCARP > 0
- || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
- !carp_ourether(ifl->ifp, eh->ether_shost))
-#endif
- ) {
+ if (bridge_ourether(ifl, eh->ether_shost)) {
m_freem(m);
return;
}
Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.327
diff -u -p -r1.327 ip_carp.c
--- netinet/ip_carp.c 12 Jan 2018 23:47:24 - 1.327
+++ netinet/ip_carp.c 25 Jan 2018 14:18:44 -
@@ -1339,12 +1348,15 @@ carp_iamatch(struct ifnet *ifp)
int
carp_ourether(struct ifnet *ifp, u_int8_t *ena)
{
- struct srpl *cif;
+ struct srpl *cif = >if_carp;
struct carp_softc *vh;
KERNEL_ASSERT_LOCKED(); /* touching if_carp + carp_vhosts */
+
+ if (SRPL_EMPTY_LOCKED(cif))
+ return (0);
+
KASSERT(ifp->if_type == IFT_ETHER);
- cif = >if_carp;
SRPL_FOREACH_LOCKED(vh, cif, sc_list) {
struct carp_vhost_entry *vhe;
Re: carp_ourether() tweak
On Mon, Jan 22, 2018 at 11:58:30AM +0100, Martin Pieuchot wrote:
> Check if `if_carp' is empty inside carp_ourether() instead of outside.
>
> ok?
Maybe I am confused by the ! and && but I think this diff changes the
logic.
Old code:
- if_carp is empty
- SRPL_EMPTY_LOCKED(>ifp->if_carp) is true
- !SRPL_EMPTY_LOCKED(>ifp->if_carp) is false
- (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
!carp_ourether(ifl->ifp, eh->ether_dhost)) is false
- if block is not executed
New Code:
- if_carp is empty
- SRPL_EMPTY_LOCKED(>ifp->if_carp) is true
- carp_ourether() returns 0
- carp_ourether(ifl->ifp, eh->ether_dhost) is false
- !carp_ourether(ifl->ifp, eh->ether_dhost) is true
- if block is executed
Is this modification of behavior intensional?
bluhm
> Index: net/if_bridge.c
> ===
> RCS file: /cvs/src/sys/net/if_bridge.c,v
> retrieving revision 1.301
> diff -u -p -r1.301 if_bridge.c
> --- net/if_bridge.c 10 Jan 2018 23:50:39 - 1.301
> +++ net/if_bridge.c 22 Jan 2018 10:54:48 -
> @@ -1108,8 +1108,7 @@ bridge_process(struct ifnet *ifp, struct
> ac = (struct arpcom *)ifl->ifp;
> if (bcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN) == 0
> #if NCARP > 0
> - || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
> - !carp_ourether(ifl->ifp, eh->ether_dhost))
> + || !carp_ourether(ifl->ifp, eh->ether_dhost)
> #endif
> ) {
> if (srcifl->bif_flags & IFBIF_LEARNING)
> @@ -1131,8 +1130,7 @@ bridge_process(struct ifnet *ifp, struct
> }
> if (bcmp(ac->ac_enaddr, eh->ether_shost, ETHER_ADDR_LEN) == 0
> #if NCARP > 0
> - || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
> - !carp_ourether(ifl->ifp, eh->ether_shost))
> + || !carp_ourether(ifl->ifp, eh->ether_shost)
> #endif
> ) {
> m_freem(m);
> Index: netinet/ip_carp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.327
> diff -u -p -r1.327 ip_carp.c
> --- netinet/ip_carp.c 12 Jan 2018 23:47:24 - 1.327
> +++ netinet/ip_carp.c 22 Jan 2018 10:53:35 -
> @@ -1339,12 +1339,15 @@ carp_iamatch(struct ifnet *ifp)
> int
> carp_ourether(struct ifnet *ifp, u_int8_t *ena)
> {
> - struct srpl *cif;
> + struct srpl *cif = >if_carp;
> struct carp_softc *vh;
>
> KERNEL_ASSERT_LOCKED(); /* touching if_carp + carp_vhosts */
> +
> + if (SRPL_EMPTY_LOCKED(cif))
> + return (0);
> +
> KASSERT(ifp->if_type == IFT_ETHER);
> - cif = >if_carp;
>
> SRPL_FOREACH_LOCKED(vh, cif, sc_list) {
> struct carp_vhost_entry *vhe;
Re: carp_ourether() tweak
ok.
friehm
On 01/22/18 11:58, Martin Pieuchot wrote:
Check if `if_carp' is empty inside carp_ourether() instead of outside.
ok?
Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.301
diff -u -p -r1.301 if_bridge.c
--- net/if_bridge.c 10 Jan 2018 23:50:39 - 1.301
+++ net/if_bridge.c 22 Jan 2018 10:54:48 -
@@ -1108,8 +1108,7 @@ bridge_process(struct ifnet *ifp, struct
ac = (struct arpcom *)ifl->ifp;
if (bcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN) == 0
#if NCARP > 0
- || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
- !carp_ourether(ifl->ifp, eh->ether_dhost))
+ || !carp_ourether(ifl->ifp, eh->ether_dhost)
#endif
) {
if (srcifl->bif_flags & IFBIF_LEARNING)
@@ -1131,8 +1130,7 @@ bridge_process(struct ifnet *ifp, struct
}
if (bcmp(ac->ac_enaddr, eh->ether_shost, ETHER_ADDR_LEN) == 0
#if NCARP > 0
- || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
- !carp_ourether(ifl->ifp, eh->ether_shost))
+ || !carp_ourether(ifl->ifp, eh->ether_shost)
#endif
) {
m_freem(m);
Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.327
diff -u -p -r1.327 ip_carp.c
--- netinet/ip_carp.c 12 Jan 2018 23:47:24 - 1.327
+++ netinet/ip_carp.c 22 Jan 2018 10:53:35 -
@@ -1339,12 +1339,15 @@ carp_iamatch(struct ifnet *ifp)
int
carp_ourether(struct ifnet *ifp, u_int8_t *ena)
{
- struct srpl *cif;
+ struct srpl *cif = >if_carp;
struct carp_softc *vh;
KERNEL_ASSERT_LOCKED(); /* touching if_carp + carp_vhosts */
+
+ if (SRPL_EMPTY_LOCKED(cif))
+ return (0);
+
KASSERT(ifp->if_type == IFT_ETHER);
- cif = >if_carp;
SRPL_FOREACH_LOCKED(vh, cif, sc_list) {
struct carp_vhost_entry *vhe;
carp_ourether() tweak
Check if `if_carp' is empty inside carp_ourether() instead of outside.
ok?
Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.301
diff -u -p -r1.301 if_bridge.c
--- net/if_bridge.c 10 Jan 2018 23:50:39 - 1.301
+++ net/if_bridge.c 22 Jan 2018 10:54:48 -
@@ -1108,8 +1108,7 @@ bridge_process(struct ifnet *ifp, struct
ac = (struct arpcom *)ifl->ifp;
if (bcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN) == 0
#if NCARP > 0
- || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
- !carp_ourether(ifl->ifp, eh->ether_dhost))
+ || !carp_ourether(ifl->ifp, eh->ether_dhost)
#endif
) {
if (srcifl->bif_flags & IFBIF_LEARNING)
@@ -1131,8 +1130,7 @@ bridge_process(struct ifnet *ifp, struct
}
if (bcmp(ac->ac_enaddr, eh->ether_shost, ETHER_ADDR_LEN) == 0
#if NCARP > 0
- || (!SRPL_EMPTY_LOCKED(>ifp->if_carp) &&
- !carp_ourether(ifl->ifp, eh->ether_shost))
+ || !carp_ourether(ifl->ifp, eh->ether_shost)
#endif
) {
m_freem(m);
Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.327
diff -u -p -r1.327 ip_carp.c
--- netinet/ip_carp.c 12 Jan 2018 23:47:24 - 1.327
+++ netinet/ip_carp.c 22 Jan 2018 10:53:35 -
@@ -1339,12 +1339,15 @@ carp_iamatch(struct ifnet *ifp)
int
carp_ourether(struct ifnet *ifp, u_int8_t *ena)
{
- struct srpl *cif;
+ struct srpl *cif = >if_carp;
struct carp_softc *vh;
KERNEL_ASSERT_LOCKED(); /* touching if_carp + carp_vhosts */
+
+ if (SRPL_EMPTY_LOCKED(cif))
+ return (0);
+
KASSERT(ifp->if_type == IFT_ETHER);
- cif = >if_carp;
SRPL_FOREACH_LOCKED(vh, cif, sc_list) {
struct carp_vhost_entry *vhe;
