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->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(&ifl->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 *)&eh->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(&ifl->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 = &ifp->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 = &ifp->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->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(&ifl->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 *)&eh->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(&ifl->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 = &ifp->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 = &ifp->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->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(&ifl->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 *)&eh->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(&ifl->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 = &ifp->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 = &ifp->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(&ifl->ifp->if_carp) is true - !SRPL_EMPTY_LOCKED(&ifl->ifp->if_carp) is false - (!SRPL_EMPTY_LOCKED(&ifl->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(&ifl->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(&ifl->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(&ifl->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 = &ifp->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 = &ifp->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(&ifl->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(&ifl->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 = &ifp->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 = &ifp->if_carp; SRPL_FOREACH_LOCKED(vh, cif, sc_list) { struct carp_vhost_entry *vhe;