On Tue, Mar 22, 2016 at 09:36:18PM +1000, David Gwynne wrote:
> this basically makes the code use if_get instead of carrying a
> pointer around. this is as mechanical as i can make it.
>
> ok?
>
> Index: if_vlan_var.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vlan_var.h,v
> retrieving revision 1.33
> diff -u -p -r1.33 if_vlan_var.h
> --- if_vlan_var.h 14 Mar 2016 03:48:47 -0000 1.33
> +++ if_vlan_var.h 22 Mar 2016 11:34:45 -0000
> @@ -61,9 +61,8 @@ struct vlan_mc_entry {
>
> struct ifvlan {
> struct arpcom ifv_ac; /* make this an interface */
> - struct ifnet *ifv_p; /* parent interface of this vlan */
> + unsigned int ifv_p; /* parent interface of this vlan */
> struct ifv_linkmib {
> - int ifvm_parent;
> u_int16_t ifvm_proto; /* encapsulation ethertype */
> u_int16_t ifvm_tag; /* tag to apply on packets leaving if */
> u_int16_t ifvm_prio; /* prio to apply on packet leaving if */
> Index: if_vlan.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vlan.c,v
> retrieving revision 1.155
> diff -u -p -r1.155 if_vlan.c
> --- if_vlan.c 18 Mar 2016 02:40:04 -0000 1.155
> +++ if_vlan.c 22 Mar 2016 11:34:45 -0000
> @@ -235,7 +235,12 @@ vlan_start(struct ifnet *ifp)
> uint8_t prio;
>
> ifv = ifp->if_softc;
> - ifp0 = ifv->ifv_p;
> + ifp0 = if_get(ifv->ifv_p);
> + if (ifp0 == NULL || (ifp0->if_flags & (IFF_UP|IFF_RUNNING)) !=
> + (IFF_UP|IFF_RUNNING)) {
> + ifq_purge(&ifp->if_snd);
You can't dereference ifp if (ifp0 == NULL)?
> + goto leave;
> + }
>
> for (;;) {
> IFQ_DEQUEUE(&ifp->if_snd, m);
> @@ -247,12 +252,6 @@ vlan_start(struct ifnet *ifp)
> bpf_mtap_ether(ifp->if_bpf, m, BPF_DIRECTION_OUT);
> #endif /* NBPFILTER > 0 */
>
> - if ((ifp0->if_flags & (IFF_UP|IFF_RUNNING)) !=
> - (IFF_UP|IFF_RUNNING)) {
> - ifp->if_oerrors++;
> - m_freem(m);
> - continue;
> - }
>
> /* IEEE 802.1p has prio 0 and 1 swapped */
> prio = m->m_pkthdr.pf.prio;
> @@ -290,6 +289,9 @@ vlan_start(struct ifnet *ifp)
> }
> ifp->if_opackets++;
> }
> +
> +leave:
> + if_put(ifp0);
> }
>
> struct mbuf *
> @@ -358,7 +360,7 @@ vlan_input(struct ifnet *ifp0, struct mb
>
> list = &tagh[TAG_HASH(tag)];
> SRPL_FOREACH(ifv, list, &i, ifv_list) {
> - if (ifp0 == ifv->ifv_p && tag == ifv->ifv_tag &&
> + if (ifp0->if_index == ifv->ifv_p && tag == ifv->ifv_tag &&
> etype == ifv->ifv_type)
> break;
> }
> @@ -417,13 +419,13 @@ vlan_config(struct ifvlan *ifv, struct i
>
> if (ifp0->if_type != IFT_ETHER)
> return EPROTONOSUPPORT;
> - if (ifv->ifv_p == ifp0 && ifv->ifv_tag == tag) /* noop */
> + if (ifp0->if_index == ifv->ifv_p && ifv->ifv_tag == tag) /* noop */
> return (0);
>
> /* Remember existing interface flags and reset the interface */
> flags = ifv->ifv_flags;
> vlan_unconfig(&ifv->ifv_if, ifp0);
> - ifv->ifv_p = ifp0;
> + ifv->ifv_p = ifp0->if_index;
> ifv->ifv_if.if_baudrate = ifp0->if_baudrate;
>
> if (ifp0->if_capabilities & IFCAP_VLAN_MTU) {
> @@ -509,8 +511,9 @@ vlan_unconfig(struct ifnet *ifp, struct
> struct ifnet *ifp0;
>
> ifv = ifp->if_softc;
> - if ((ifp0 = ifv->ifv_p) == NULL)
> - return 0;
> + ifp0 = if_get(ifv->ifv_p);
> + if (ifp0 == NULL)
> + goto disconnect;
>
> /* Unset promisc mode on the interface and its parent */
> if (ifv->ifv_flags & IFVF_PROMISC) {
> @@ -544,8 +547,9 @@ vlan_unconfig(struct ifnet *ifp, struct
> */
> vlan_multi_apply(ifv, ifp0, SIOCDELMULTI);
>
> +disconnect:
> /* Disconnect from parent. */
> - ifv->ifv_p = NULL;
> + ifv->ifv_p = 0;
> ifv->ifv_if.if_mtu = ETHERMTU;
> ifv->ifv_if.if_hardmtu = ETHERMTU;
> ifv->ifv_flags = 0;
So you're changing error code path here? I'm not sure this correctness
but I'd change this separately.
> @@ -557,6 +561,8 @@ vlan_unconfig(struct ifnet *ifp, struct
> bzero(LLADDR(sdl), ETHER_ADDR_LEN);
> bzero(ifv->ifv_ac.ac_enaddr, ETHER_ADDR_LEN);
>
> + if_put(ifp0);
> +
> return (0);
> }
>
> @@ -564,12 +570,22 @@ void
> vlan_vlandev_state(void *v)
> {
> struct ifvlan *ifv = v;
> + struct ifnet *ifp0;
> + int link_state = LINK_STATE_DOWN;
> + uint64_t baudrate = 0;
>
> - if (ifv->ifv_if.if_link_state == ifv->ifv_p->if_link_state)
> + ifp0 = if_get(ifv->ifv_p);
> + if (ifp0 != NULL) {
> + link_state = ifp0->if_link_state;
> + baudrate = ifp0->if_baudrate;
> + }
> + if_put(ifp0);
> +
> + if (ifv->ifv_if.if_link_state == link_state)
> return;
>
> - ifv->ifv_if.if_link_state = ifv->ifv_p->if_link_state;
> - ifv->ifv_if.if_baudrate = ifv->ifv_p->if_baudrate;
> + ifv->ifv_if.if_link_state = link_state;
> + ifv->ifv_if.if_baudrate = baudrate;
> if_link_state_change(&ifv->ifv_if);
> }
>
I don't follow the logic here...
