diff mbox series

[net-next,1/2] selftests: openvswitch: fix action formatting

Message ID 20240603183121.2305013-1-amorenoz@redhat.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next,1/2] selftests: openvswitch: fix action formatting | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 67 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-04--00-00 (tests: 1040)

Commit Message

Adrián Moreno June 3, 2024, 6:31 p.m. UTC 
In the action formatting function ("dpstr"), the iteration is made over
the nla_map, so if there are more than one attribute from the same type
we only print the first one.

Fix this by iterating over the actual attributes.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 .../selftests/net/openvswitch/ovs-dpctl.py    | 48 +++++++++++--------
 1 file changed, 27 insertions(+), 21 deletions(-)

Comments

Aaron Conole June 3, 2024, 7 p.m. UTC | #1 
Adrian Moreno <amorenoz@redhat.com> writes:

> In the action formatting function ("dpstr"), the iteration is made over
> the nla_map, so if there are more than one attribute from the same type
> we only print the first one.
>
> Fix this by iterating over the actual attributes.
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  .../selftests/net/openvswitch/ovs-dpctl.py    | 48 +++++++++++--------
>  1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 1dd057afd3fb..b76907ac0092 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -437,40 +437,46 @@ class ovsactions(nla):
>      def dpstr(self, more=False):
>          print_str = ""
>  
> -        for field in self.nla_map:
> -            if field[1] == "none" or self.get_attr(field[0]) is None:
> +        for attr_name, value in self["attrs"]:
> +            attr_desc = next(filter(lambda x: x[0] == attr_name, self.nla_map),
> +                             None)
> +            if not attr_desc:
> +                raise ValueError("Unknown attribute: %s" % attr)
> +
> +            attr_type = attr_desc[1]
> +
> +            if attr_type == "none":

I agree, this is an issue.  BUT I think it might be better to just
filter by field type up front.  See:

https://github.com/apconole/linux-next-work/commit/7262107de7170d44b6dbf6c5ea6f7e6c0bb71d36#diff-3e72e7405c6bb4e9842bed5f63883ca930387086bb40d4034e92ed83a5decb4bR441

That version I think ends up being much easier to follow.  If you want
to take it for your series, feel free.  If you disagree, maybe there's
something I'm not considering about it.

NOTE that version is just a bunch of independent changes that are
squashed together.  I have a cleaner version.

I can also bundle up the series I have so far and submit, but I didn't
want to do that until I got all the pmtu.sh support working.  Maybe it
makes sense to send it now though.  Simon, Jakub - wdyt?

>                  continue
>              if print_str != "":
>                  print_str += ","
>  
> -            if field[1] == "uint32":
> -                if field[0] == "OVS_ACTION_ATTR_OUTPUT":
> -                    print_str += "%d" % int(self.get_attr(field[0]))
> -                elif field[0] == "OVS_ACTION_ATTR_RECIRC":
> -                    print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
> -                elif field[0] == "OVS_ACTION_ATTR_TRUNC":
> -                    print_str += "trunc(%d)" % int(self.get_attr(field[0]))
> -                elif field[0] == "OVS_ACTION_ATTR_DROP":
> -                    print_str += "drop(%d)" % int(self.get_attr(field[0]))
> -            elif field[1] == "flag":
> -                if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
> +            if attr_type == "uint32":
> +                if attr_name == "OVS_ACTION_ATTR_OUTPUT":
> +                    print_str += "%d" % int(value)
> +                elif attr_name == "OVS_ACTION_ATTR_RECIRC":
> +                    print_str += "recirc(0x%x)" % int(value)
> +                elif attr_name == "OVS_ACTION_ATTR_TRUNC":
> +                    print_str += "trunc(%d)" % int(value)
> +                elif attr_name == "OVS_ACTION_ATTR_DROP":
> +                    print_str += "drop(%d)" % int(value)
> +            elif attr_type == "flag":
> +                if attr_name == "OVS_ACTION_ATTR_CT_CLEAR":
>                      print_str += "ct_clear"
> -                elif field[0] == "OVS_ACTION_ATTR_POP_VLAN":
> +                elif attr_name == "OVS_ACTION_ATTR_POP_VLAN":
>                      print_str += "pop_vlan"
> -                elif field[0] == "OVS_ACTION_ATTR_POP_ETH":
> +                elif attr_name == "OVS_ACTION_ATTR_POP_ETH":
>                      print_str += "pop_eth"
> -                elif field[0] == "OVS_ACTION_ATTR_POP_NSH":
> +                elif attr_name == "OVS_ACTION_ATTR_POP_NSH":
>                      print_str += "pop_nsh"
> -                elif field[0] == "OVS_ACTION_ATTR_POP_MPLS":
> +                elif attr_name == "OVS_ACTION_ATTR_POP_MPLS":
>                      print_str += "pop_mpls"
>              else:
> -                datum = self.get_attr(field[0])
> -                if field[0] == "OVS_ACTION_ATTR_CLONE":
> +                if attr_name == "OVS_ACTION_ATTR_CLONE":
>                      print_str += "clone("
> -                    print_str += datum.dpstr(more)
> +                    print_str += value.dpstr(more)
>                      print_str += ")"
>                  else:
> -                    print_str += datum.dpstr(more)
> +                    print_str += value.dpstr(more)
>  
>          return print_str
Jakub Kicinski June 4, 2024, 12:15 a.m. UTC | #2 
On Mon, 03 Jun 2024 15:00:03 -0400 Aaron Conole wrote:
> I agree, this is an issue.  BUT I think it might be better to just
> filter by field type up front.  See:
> 
> https://github.com/apconole/linux-next-work/commit/7262107de7170d44b6dbf6c5ea6f7e6c0bb71d36#diff-3e72e7405c6bb4e9842bed5f63883ca930387086bb40d4034e92ed83a5decb4bR441
> 
> That version I think ends up being much easier to follow.  If you want
> to take it for your series, feel free.  If you disagree, maybe there's
> something I'm not considering about it.
> 
> NOTE that version is just a bunch of independent changes that are
> squashed together.  I have a cleaner version.
> 
> I can also bundle up the series I have so far and submit, but I didn't
> want to do that until I got all the pmtu.sh support working.  Maybe it
> makes sense to send it now though.  Simon, Jakub - wdyt?

I'd say - hold onto the changes until pmtu.sh works, unless there's
*any* reason for a particular patch to go in early, eg:
 - patch fixes existing bug
 - someone else needs a patch
 - ...
 - a patch which falls under some of the criteria above depends 
   on the patch
Adrián Moreno June 6, 2024, 9:05 a.m. UTC | #3 
On Mon, Jun 03, 2024 at 03:00:03PM GMT, Aaron Conole wrote:
> Adrian Moreno <amorenoz@redhat.com> writes:
>
> > In the action formatting function ("dpstr"), the iteration is made over
> > the nla_map, so if there are more than one attribute from the same type
> > we only print the first one.
> >
> > Fix this by iterating over the actual attributes.
> >
> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > ---
> >  .../selftests/net/openvswitch/ovs-dpctl.py    | 48 +++++++++++--------
> >  1 file changed, 27 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > index 1dd057afd3fb..b76907ac0092 100644
> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > @@ -437,40 +437,46 @@ class ovsactions(nla):
> >      def dpstr(self, more=False):
> >          print_str = ""
> >
> > -        for field in self.nla_map:
> > -            if field[1] == "none" or self.get_attr(field[0]) is None:
> > +        for attr_name, value in self["attrs"]:
> > +            attr_desc = next(filter(lambda x: x[0] == attr_name, self.nla_map),
> > +                             None)
> > +            if not attr_desc:
> > +                raise ValueError("Unknown attribute: %s" % attr)
> > +
> > +            attr_type = attr_desc[1]
> > +
> > +            if attr_type == "none":
>
> I agree, this is an issue.  BUT I think it might be better to just
> filter by field type up front.  See:
>
> https://github.com/apconole/linux-next-work/commit/7262107de7170d44b6dbf6c5ea6f7e6c0bb71d36#diff-3e72e7405c6bb4e9842bed5f63883ca930387086bb40d4034e92ed83a5decb4bR441
>
> That version I think ends up being much easier to follow.  If you want
> to take it for your series, feel free.  If you disagree, maybe there's
> something I'm not considering about it.
>

I agree. It's better to check field attribute names first. I found this
during manual testing of the "emit_sample" series but I ended up not
needing it for the automated one, so I'm OK waiting for your cleanup
series.

In fact, I also have some patches that try some rework of this part. In
particular, I tried to unify all attributes under a common base class
that would handle printing and parsing. That way, most cases would fall
into "print_str += datum.dpstr(more)" and the "if/elif" block would
shrink significantly.

> NOTE that version is just a bunch of independent changes that are
> squashed together.  I have a cleaner version.
>
> I can also bundle up the series I have so far and submit, but I didn't
> want to do that until I got all the pmtu.sh support working.  Maybe it
> makes sense to send it now though.  Simon, Jakub - wdyt?
>
> >                  continue
> >              if print_str != "":
> >                  print_str += ","
> >
> > -            if field[1] == "uint32":
> > -                if field[0] == "OVS_ACTION_ATTR_OUTPUT":
> > -                    print_str += "%d" % int(self.get_attr(field[0]))
> > -                elif field[0] == "OVS_ACTION_ATTR_RECIRC":
> > -                    print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
> > -                elif field[0] == "OVS_ACTION_ATTR_TRUNC":
> > -                    print_str += "trunc(%d)" % int(self.get_attr(field[0]))
> > -                elif field[0] == "OVS_ACTION_ATTR_DROP":
> > -                    print_str += "drop(%d)" % int(self.get_attr(field[0]))
> > -            elif field[1] == "flag":
> > -                if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
> > +            if attr_type == "uint32":
> > +                if attr_name == "OVS_ACTION_ATTR_OUTPUT":
> > +                    print_str += "%d" % int(value)
> > +                elif attr_name == "OVS_ACTION_ATTR_RECIRC":
> > +                    print_str += "recirc(0x%x)" % int(value)
> > +                elif attr_name == "OVS_ACTION_ATTR_TRUNC":
> > +                    print_str += "trunc(%d)" % int(value)
> > +                elif attr_name == "OVS_ACTION_ATTR_DROP":
> > +                    print_str += "drop(%d)" % int(value)
> > +            elif attr_type == "flag":
> > +                if attr_name == "OVS_ACTION_ATTR_CT_CLEAR":
> >                      print_str += "ct_clear"
> > -                elif field[0] == "OVS_ACTION_ATTR_POP_VLAN":
> > +                elif attr_name == "OVS_ACTION_ATTR_POP_VLAN":
> >                      print_str += "pop_vlan"
> > -                elif field[0] == "OVS_ACTION_ATTR_POP_ETH":
> > +                elif attr_name == "OVS_ACTION_ATTR_POP_ETH":
> >                      print_str += "pop_eth"
> > -                elif field[0] == "OVS_ACTION_ATTR_POP_NSH":
> > +                elif attr_name == "OVS_ACTION_ATTR_POP_NSH":
> >                      print_str += "pop_nsh"
> > -                elif field[0] == "OVS_ACTION_ATTR_POP_MPLS":
> > +                elif attr_name == "OVS_ACTION_ATTR_POP_MPLS":
> >                      print_str += "pop_mpls"
> >              else:
> > -                datum = self.get_attr(field[0])
> > -                if field[0] == "OVS_ACTION_ATTR_CLONE":
> > +                if attr_name == "OVS_ACTION_ATTR_CLONE":
> >                      print_str += "clone("
> > -                    print_str += datum.dpstr(more)
> > +                    print_str += value.dpstr(more)
> >                      print_str += ")"
> >                  else:
> > -                    print_str += datum.dpstr(more)
> > +                    print_str += value.dpstr(more)
> >
> >          return print_str
>
Aaron Conole June 11, 2024, 2:04 p.m. UTC | #4 
Adrián Moreno <amorenoz@redhat.com> writes:

> On Mon, Jun 03, 2024 at 03:00:03PM GMT, Aaron Conole wrote:
>> Adrian Moreno <amorenoz@redhat.com> writes:
>>
>> > In the action formatting function ("dpstr"), the iteration is made over
>> > the nla_map, so if there are more than one attribute from the same type
>> > we only print the first one.
>> >
>> > Fix this by iterating over the actual attributes.
>> >
>> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> > ---
>> >  .../selftests/net/openvswitch/ovs-dpctl.py    | 48 +++++++++++--------
>> >  1 file changed, 27 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > index 1dd057afd3fb..b76907ac0092 100644
>> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > @@ -437,40 +437,46 @@ class ovsactions(nla):
>> >      def dpstr(self, more=False):
>> >          print_str = ""
>> >
>> > -        for field in self.nla_map:
>> > -            if field[1] == "none" or self.get_attr(field[0]) is None:
>> > +        for attr_name, value in self["attrs"]:
>> > +            attr_desc = next(filter(lambda x: x[0] == attr_name, self.nla_map),
>> > +                             None)
>> > +            if not attr_desc:
>> > +                raise ValueError("Unknown attribute: %s" % attr)
>> > +
>> > +            attr_type = attr_desc[1]
>> > +
>> > +            if attr_type == "none":
>>
>> I agree, this is an issue.  BUT I think it might be better to just
>> filter by field type up front.  See:
>>
>> https://github.com/apconole/linux-next-work/commit/7262107de7170d44b6dbf6c5ea6f7e6c0bb71d36#diff-3e72e7405c6bb4e9842bed5f63883ca930387086bb40d4034e92ed83a5decb4bR441
>>
>> That version I think ends up being much easier to follow.  If you want
>> to take it for your series, feel free.  If you disagree, maybe there's
>> something I'm not considering about it.
>>
>
> I agree. It's better to check field attribute names first. I found this
> during manual testing of the "emit_sample" series but I ended up not
> needing it for the automated one, so I'm OK waiting for your cleanup
> series.

I'll get stuff out this week for it.

> In fact, I also have some patches that try some rework of this part. In
> particular, I tried to unify all attributes under a common base class
> that would handle printing and parsing. That way, most cases would fall
> into "print_str += datum.dpstr(more)" and the "if/elif" block would
> shrink significantly.

That sounds very good.

>> NOTE that version is just a bunch of independent changes that are
>> squashed together.  I have a cleaner version.
>>
>> I can also bundle up the series I have so far and submit, but I didn't
>> want to do that until I got all the pmtu.sh support working.  Maybe it
>> makes sense to send it now though.  Simon, Jakub - wdyt?
>>
>> >                  continue
>> >              if print_str != "":
>> >                  print_str += ","
>> >
>> > -            if field[1] == "uint32":
>> > -                if field[0] == "OVS_ACTION_ATTR_OUTPUT":
>> > -                    print_str += "%d" % int(self.get_attr(field[0]))
>> > -                elif field[0] == "OVS_ACTION_ATTR_RECIRC":
>> > -                    print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
>> > -                elif field[0] == "OVS_ACTION_ATTR_TRUNC":
>> > -                    print_str += "trunc(%d)" % int(self.get_attr(field[0]))
>> > -                elif field[0] == "OVS_ACTION_ATTR_DROP":
>> > -                    print_str += "drop(%d)" % int(self.get_attr(field[0]))
>> > -            elif field[1] == "flag":
>> > -                if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
>> > +            if attr_type == "uint32":
>> > +                if attr_name == "OVS_ACTION_ATTR_OUTPUT":
>> > +                    print_str += "%d" % int(value)
>> > +                elif attr_name == "OVS_ACTION_ATTR_RECIRC":
>> > +                    print_str += "recirc(0x%x)" % int(value)
>> > +                elif attr_name == "OVS_ACTION_ATTR_TRUNC":
>> > +                    print_str += "trunc(%d)" % int(value)
>> > +                elif attr_name == "OVS_ACTION_ATTR_DROP":
>> > +                    print_str += "drop(%d)" % int(value)
>> > +            elif attr_type == "flag":
>> > +                if attr_name == "OVS_ACTION_ATTR_CT_CLEAR":
>> >                      print_str += "ct_clear"
>> > -                elif field[0] == "OVS_ACTION_ATTR_POP_VLAN":
>> > +                elif attr_name == "OVS_ACTION_ATTR_POP_VLAN":
>> >                      print_str += "pop_vlan"
>> > -                elif field[0] == "OVS_ACTION_ATTR_POP_ETH":
>> > +                elif attr_name == "OVS_ACTION_ATTR_POP_ETH":
>> >                      print_str += "pop_eth"
>> > -                elif field[0] == "OVS_ACTION_ATTR_POP_NSH":
>> > +                elif attr_name == "OVS_ACTION_ATTR_POP_NSH":
>> >                      print_str += "pop_nsh"
>> > -                elif field[0] == "OVS_ACTION_ATTR_POP_MPLS":
>> > +                elif attr_name == "OVS_ACTION_ATTR_POP_MPLS":
>> >                      print_str += "pop_mpls"
>> >              else:
>> > -                datum = self.get_attr(field[0])
>> > -                if field[0] == "OVS_ACTION_ATTR_CLONE":
>> > +                if attr_name == "OVS_ACTION_ATTR_CLONE":
>> >                      print_str += "clone("
>> > -                    print_str += datum.dpstr(more)
>> > +                    print_str += value.dpstr(more)
>> >                      print_str += ")"
>> >                  else:
>> > -                    print_str += datum.dpstr(more)
>> > +                    print_str += value.dpstr(more)
>> >
>> >          return print_str
>>
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 1dd057afd3fb..b76907ac0092 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -437,40 +437,46 @@  class ovsactions(nla):
     def dpstr(self, more=False):
         print_str = ""
 
-        for field in self.nla_map:
-            if field[1] == "none" or self.get_attr(field[0]) is None:
+        for attr_name, value in self["attrs"]:
+            attr_desc = next(filter(lambda x: x[0] == attr_name, self.nla_map),
+                             None)
+            if not attr_desc:
+                raise ValueError("Unknown attribute: %s" % attr)
+
+            attr_type = attr_desc[1]
+
+            if attr_type == "none":
                 continue
             if print_str != "":
                 print_str += ","
 
-            if field[1] == "uint32":
-                if field[0] == "OVS_ACTION_ATTR_OUTPUT":
-                    print_str += "%d" % int(self.get_attr(field[0]))
-                elif field[0] == "OVS_ACTION_ATTR_RECIRC":
-                    print_str += "recirc(0x%x)" % int(self.get_attr(field[0]))
-                elif field[0] == "OVS_ACTION_ATTR_TRUNC":
-                    print_str += "trunc(%d)" % int(self.get_attr(field[0]))
-                elif field[0] == "OVS_ACTION_ATTR_DROP":
-                    print_str += "drop(%d)" % int(self.get_attr(field[0]))
-            elif field[1] == "flag":
-                if field[0] == "OVS_ACTION_ATTR_CT_CLEAR":
+            if attr_type == "uint32":
+                if attr_name == "OVS_ACTION_ATTR_OUTPUT":
+                    print_str += "%d" % int(value)
+                elif attr_name == "OVS_ACTION_ATTR_RECIRC":
+                    print_str += "recirc(0x%x)" % int(value)
+                elif attr_name == "OVS_ACTION_ATTR_TRUNC":
+                    print_str += "trunc(%d)" % int(value)
+                elif attr_name == "OVS_ACTION_ATTR_DROP":
+                    print_str += "drop(%d)" % int(value)
+            elif attr_type == "flag":
+                if attr_name == "OVS_ACTION_ATTR_CT_CLEAR":
                     print_str += "ct_clear"
-                elif field[0] == "OVS_ACTION_ATTR_POP_VLAN":
+                elif attr_name == "OVS_ACTION_ATTR_POP_VLAN":
                     print_str += "pop_vlan"
-                elif field[0] == "OVS_ACTION_ATTR_POP_ETH":
+                elif attr_name == "OVS_ACTION_ATTR_POP_ETH":
                     print_str += "pop_eth"
-                elif field[0] == "OVS_ACTION_ATTR_POP_NSH":
+                elif attr_name == "OVS_ACTION_ATTR_POP_NSH":
                     print_str += "pop_nsh"
-                elif field[0] == "OVS_ACTION_ATTR_POP_MPLS":
+                elif attr_name == "OVS_ACTION_ATTR_POP_MPLS":
                     print_str += "pop_mpls"
             else:
-                datum = self.get_attr(field[0])
-                if field[0] == "OVS_ACTION_ATTR_CLONE":
+                if attr_name == "OVS_ACTION_ATTR_CLONE":
                     print_str += "clone("
-                    print_str += datum.dpstr(more)
+                    print_str += value.dpstr(more)
                     print_str += ")"
                 else:
-                    print_str += datum.dpstr(more)
+                    print_str += value.dpstr(more)
 
         return print_str