| Message ID | 20240927112824.339-1-kac.ludwinski@icloud.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] selftests: forwarding: no_forwarding: Fix issue related with assigning two different vids to the same interface. | expand |
Hi Kacper, On Fri, Sep 27, 2024 at 08:28:24PM +0900, Kacper Ludwinski wrote: > Fix typo. > Currently, the second bridge command overwrites the first one. > Fix this by adding this VID to the interface behind $swp2. > > Fixes: 476a4f05d9b8 ("selftests: forwarding: add a no_forwarding.sh test") > Signed-off-by: Kacper Ludwinski <kac.ludwinski@icloud.com> > --- > tools/testing/selftests/net/forwarding/no_forwarding.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/net/forwarding/no_forwarding.sh b/tools/testing/selftests/net/forwarding/no_forwarding.sh > index 9e677aa64a06..694ece9ba3a7 100755 > --- a/tools/testing/selftests/net/forwarding/no_forwarding.sh > +++ b/tools/testing/selftests/net/forwarding/no_forwarding.sh > @@ -202,7 +202,7 @@ one_bridge_two_pvids() > ip link set $swp2 master br0 > > bridge vlan add dev $swp1 vid 1 pvid untagged > - bridge vlan add dev $swp1 vid 2 pvid untagged > + bridge vlan add dev $swp2 vid 2 pvid untagged > > run_test "Switch ports in VLAN-aware bridge with different PVIDs" > > -- > 2.43.0 > Thank you for the patch. More process problems though. Please use the space under "---" to write a revision history for the patches. I had to download your v2 and v3 to figure out that you changed the Signed-off-by email address and fixed some whitespaces. Generally it is the task of the patch submitter, not the reviewers, to record the differences. Annotating the revision history to links on lore.kernel.org of previous patch submissions is also a sign of a well cared-for patch. Secondly, Hangbin Liu left you a Reviewed-by: tag in a previous patch submission. https://lore.kernel.org/netdev/ZvTDJyKatBVpgqMj@fedora/ When you resubmit, it is your duty to pick up such tags from previous patch versions and keep them in your commit message. Some say to keep your Signed-off-by: tag as the final line though. No empty lines between tags (Fixes:, Reviewed-by:, Signed-off-by: etc). Thirdly, the commit title is unnecessarily long. Please limit it to around 80 characters, and only exceed if you cannot abbreviate anymore. For example: selftests: net: no_forwarding: fix VID for swp2 in one_bridge_two_pvids() test Fourth, bug fixes as per the Documentation/process/stable-kernel-rules.rst process must be real bugs with a visible impact. So your commit message has to make it clear what that impact is, to pass the 'net' vs 'net-next' triage. You can't leave that part for the reader to figure it out. For example, I suggest this for the commit message body: The one_bridge_two_pvids() test intends to check that there is no leakage of traffic between bridge ports which have a single VLAN - the PVID VLAN. Because of a typo, port $swp1 is configured with a PVID twice (second command overwrites first), and $swp2 isn't configured at all (and since the bridge vlan_default_pvid property is set to 0, this port will not have a PVID at all, so it will drop all untagged and priority-tagged traffic). So, instead of testing the configuration that was intended, we are testing a different one, where one port has PVID 2 and the other has no PVID. This incorrect version of the test should also pass, but is ineffective for its purpose, so fix the typo. This test was run on $<insert platform here> and this is the result: <insert kselftest output here>. Please resubmit with these changes. Thanks. pw-bot: changes-requested
diff --git a/tools/testing/selftests/net/forwarding/no_forwarding.sh b/tools/testing/selftests/net/forwarding/no_forwarding.sh index 9e677aa64a06..694ece9ba3a7 100755 --- a/tools/testing/selftests/net/forwarding/no_forwarding.sh +++ b/tools/testing/selftests/net/forwarding/no_forwarding.sh @@ -202,7 +202,7 @@ one_bridge_two_pvids() ip link set $swp2 master br0 bridge vlan add dev $swp1 vid 1 pvid untagged - bridge vlan add dev $swp1 vid 2 pvid untagged + bridge vlan add dev $swp2 vid 2 pvid untagged run_test "Switch ports in VLAN-aware bridge with different PVIDs"
Fix typo. Currently, the second bridge command overwrites the first one. Fix this by adding this VID to the interface behind $swp2. Fixes: 476a4f05d9b8 ("selftests: forwarding: add a no_forwarding.sh test") Signed-off-by: Kacper Ludwinski <kac.ludwinski@icloud.com> --- tools/testing/selftests/net/forwarding/no_forwarding.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)