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

[der Mouse <mouse%Rodents-Montreal.ORG@localhost>]

>> +            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.

Er.  Yes, it should skip...hmm, that's odd; I had debugging printfs in
there at one point, without such a test, but didn't see it re-enable
untaggedvlan.  I wonder why.  *rummage* *rummage*  Ah, because I
provoked the removal with ifconfig destroy, which does a LIST_REMOVE
before calling vlan_unconfig, so ifv wasn't in the list at that point.

I'll add a suitable check.

> Please use LIST_FOREACH(). :-)

I just copied the loop framework from elsewhere; I don't like the
LIST_* macros, so I don't use them myself and thus don't know the
facilities they provide.  I held my nose and used them here for the
same reason I held my nose and copied the other ugly style aspects:
uniformly ugly style is better than inconsistent style (well, perhaps
except for really extreme cases, much more extreme than this).

I can switch it to LIST_FOREACH if you like; how would that loop be
written with 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?

I'm not sure.  On input, only one of them will get input packets.  The
only use I can see for it is merging output streams when either you
don't care about input or it doesn't matter which interface input goes
to.  But I see no particular reason to prevent it, and the former code
didn't prevent it either, for tagged vlans....

When I added that code, I was thinking something like "this probably
shouldn't happen, but nothing's preventing it, and just because I can't
think of a use for it offhand is no excuse to misbehave in that case".
I probably should have added a comment to that effect.

>> [...]
> Now the patch becomes very hard to read because you have shifted
> everything over.

Yes; it was intended to be applied, not read.  If you want something
intended to be read, apply it to a scratch copy of the file and then
use diff -u -b. :-)

/~\ The ASCII                           der Mouse
\ / Ribbon Campaign
 X  Against HTML                mouse%rodents-montreal.org@localhost
/ \ Email!           7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B