Re: bridges, vlans, and xen, oh my!

[David Young <dyoung%pobox.com@localhost>]

On Sat, Jun 21, 2008 at 01:23:29AM -0400, der Mouse wrote:
> @@ -368,6 +373,7 @@
>  vlan_unconfig(struct ifnet *ifp)
>  {
>       struct ifvlan *ifv = ifp->if_softc;
> +     struct ifvlan *ifv2;
>  
>       if (ifv->ifv_p == NULL)
>               return;
> @@ -399,6 +405,17 @@
>                       }
>               }
>  
> +             if (ifv->ifv_tag == EVL_UNTAGGED) {
> +                     ec->ec_untaggedvlan = 0;
> +                     for (ifv2 = LIST_FIRST(&ifv_list); ifv2 != NULL;
> +                         ifv2 = LIST_NEXT(ifv2, ifv_list))
> +                             if ( (ifv2->ifv_p == ifv->ifv_p) &&
> +                                  (ifv2->ifv_tag == EVL_UNTAGGED) ) {
> +                                     ec->ec_untaggedvlan = 1;
> +                                     break;
> +                             }
> +             }
> +

This is confusing.  If I understand correctly, there may be only one
untagged VLAN active on any parent ifnet, and this code activates a second
untagged VLAN if the first is deactivated.  I think that the for-loop
over the ifvs deserves a comment as to why it is unnecessary to skip
over ifv2 if ifv2 == ifv, or else it should explicitly skip.

Please use LIST_FOREACH(). :-)

Is it a good idea to allow more than one VLAN with the same tag, or more
than one untagged VLAN to be configured on the same parent?

> @@ -738,85 +755,90 @@
>                       bpf_mtap(ifp->if_bpf, m);
>  #endif
>               /*
> -              * If the parent can insert the tag itself, just mark
> -              * the tag in the mbuf header.
> +              * EVL_UNTAGGED means "don't tag" on output.
>                */
> -             if (ec->ec_capabilities & ETHERCAP_VLAN_HWTAGGING) {
> -                     struct m_tag *mtag;
> -
> -                     mtag = m_tag_get(PACKET_TAG_VLAN, sizeof(u_int),
> -                         M_NOWAIT);
> -                     if (mtag == NULL) {
> -                             ifp->if_oerrors++;
> -                             m_freem(m);
> -                             continue;
> -                     }
> -                     *(u_int *)(mtag + 1) = ifv->ifv_tag;
> -                     m_tag_prepend(m, mtag);
> -             } else {
> +             if (ifv->ifv_tag != EVL_UNTAGGED) {

Now the patch becomes very hard to read because you have shifted
everything over.  vlan_start already has too many levels of indentation.
You can accomplish the same thing by extracting a subroutine, or else
like this,

        if (ifv->ifv_tag == EVLUNTAGGED)
                ;
        else if (ec->ec_capabilities & ETHERCAP_VLAN_HWTAGGING) {

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 278-3933 ext 24