On Mon, Apr 11, 2022 at 20:21, Vladimir Oltean <vladimir.oltean@xxxxxxx> wrote:
> On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote:
>> +# Verify per-port flood control flags of unknown BUM traffic.
>> +#
>> +# br0
>> +# / \
>> +# h1 h2
>
> I think the picture is slightly inaccurate. From it I understand that h1
> and h2 are bridge ports, but they are stations attached to the real
> bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces.
Hmm, yeah either that or drop it entirely. I sort of assumed everyone
knew about the h<-[veth]->swp (or actual cable) setup, but you're right
this is a bit unclear. Me and Tobias have internally used h<-->p (for
host<-->bridge-port) and other similar nomenclature. Finding a good
name that fits easily, and is still readable, in ASCII drawings is hard.
I'll give it a go in the next drop, thanks!
>> +#set -x
> stray debug line
thx
>> +# Disable promisc to ensure we only receive flooded frames
>> +export TCPDUMP_EXTRA_FLAGS="-pl"
> Exporting should be required only for sub-shells, doesn't apply when you
> source a script.
Ah thanks, will fix!
>> +# Port mappings and flood flag pattern to set/detect
>> +declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2)
> Maybe you could populate the "ports" and the "flagN" arrays in the same
> order, i.e. bridge first for all?
Good point, thanks!
> Also, to be honest, a generic name like "ports" is hard to digest,
> especially since you have another generic variable name "iface".
> Maybe "brports" and "station" is a little bit more specific?
Is there a common naming standard between bridge tests, or is it more
important to be consistent the test overview (test heading w/ picture)?
Anyway, I'll have a look at the naming for the next drop.
>> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
>> +declare -A flag2=([$swp1]=off [$swp2]=on [br0]=off)
>> +declare -A flag3=([$swp1]=off [$swp2]=on [br0]=on )
>> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )
> If it's not too much, maybe these could be called "flags_pass1", etc.
> Again, it was a bit hard to digest on first read.
More like flags_pass_fail, but since its the flooding flags, maybe
flood_patternN would be better?
>> +do_flood_unknown()
>> +{
>> + local type=$1
>> + local pass=$2
>> + local flag=$3
>> + local pkt=$4
>> + local -n flags=$5
> I find it slightly less confusing if "flag" and "flags" are next to each
> other in the parameter list, since they're related.
Hmm, OK.
>> +# echo "Dumping PCAP from $iface, expecting ${flags[$port]}:"
>> +# tcpdump_show $iface
> Do something about the commented lines.
Oups, thanks!
>> + tcpdump_show $iface |grep -q "$SRC_MAC"
> Space between pipe and grep.
Will fix!
>> + check_err_fail "${flags[$port]} = on" $? "failed flooding from $h1 to port $port"
> I think the "failed" word here is superfluous, since check_err_fail
> already says "$what succeeded, but should have failed".
Ah, good point!
Thank you for the review! <3
/J
