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

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

[Adrian Moreno]

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":
                 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
 
-- 
2.45.1

[PATCH net-next 2/2] selftests: openvswitch: set value to nla flags

[Adrian Moreno]

Netlink flags, although they don't have payload at the netlink level,
are represented as having a "True" value in pyroute2.

Without it, trying to add a flow with a flag-type action (e.g: pop_vlan)
fails with the following traceback:

Traceback (most recent call last):
  File "[...]/ovs-dpctl.py", line 2498, in <module>
    sys.exit(main(sys.argv))
             ^^^^^^^^^^^^^^
  File "[...]/ovs-dpctl.py", line 2487, in main
    ovsflow.add_flow(rep["dpifindex"], flow)
  File "[...]/ovs-dpctl.py", line 2136, in add_flow
    reply = self.nlm_request(
            ^^^^^^^^^^^^^^^^^
  File "[...]/pyroute2/netlink/nlsocket.py", line 822, in nlm_request
    return tuple(self._genlm_request(*argv, **kwarg))
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "[...]/pyroute2/netlink/generic/__init__.py", line 126, in
nlm_request
    return tuple(super().nlm_request(*argv, **kwarg))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "[...]/pyroute2/netlink/nlsocket.py", line 1124, in nlm_request
    self.put(msg, msg_type, msg_flags, msg_seq=msg_seq)
  File "[...]/pyroute2/netlink/nlsocket.py", line 389, in put
    self.sendto_gate(msg, addr)
  File "[...]/pyroute2/netlink/nlsocket.py", line 1056, in sendto_gate
    msg.encode()
  File "[...]/pyroute2/netlink/__init__.py", line 1245, in encode
    offset = self.encode_nlas(offset)
             ^^^^^^^^^^^^^^^^^^^^^^^^
  File "[...]/pyroute2/netlink/__init__.py", line 1560, in encode_nlas
    nla_instance.setvalue(cell[1])
  File "[...]/pyroute2/netlink/__init__.py", line 1265, in setvalue
    nlv.setvalue(nla_tuple[1])
                 ~~~~~~~~~^^^
IndexError: list index out of range

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index b76907ac0092..a2395c3f37a1 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -537,7 +537,7 @@ class ovsactions(nla):
             for flat_act in parse_flat_map:
                 if parse_starts_block(actstr, flat_act[0], False):
                     actstr = actstr[len(flat_act[0]):]
-                    self["attrs"].append([flat_act[1]])
+                    self["attrs"].append([flat_act[1], True])
                     actstr = actstr[strspn(actstr, ", ") :]
                     parsed = True
 
-- 
2.45.1

Re: [PATCH net-next 2/2] selftests: openvswitch: set value to nla flags

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> Netlink flags, although they don't have payload at the netlink level,
> are represented as having a "True" value in pyroute2.
>
> Without it, trying to add a flow with a flag-type action (e.g: pop_vlan)
> fails with the following traceback:
>
> Traceback (most recent call last):
>   File "[...]/ovs-dpctl.py", line 2498, in <module>
>     sys.exit(main(sys.argv))
>              ^^^^^^^^^^^^^^
>   File "[...]/ovs-dpctl.py", line 2487, in main
>     ovsflow.add_flow(rep["dpifindex"], flow)
>   File "[...]/ovs-dpctl.py", line 2136, in add_flow
>     reply = self.nlm_request(
>             ^^^^^^^^^^^^^^^^^
>   File "[...]/pyroute2/netlink/nlsocket.py", line 822, in nlm_request
>     return tuple(self._genlm_request(*argv, **kwarg))
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File "[...]/pyroute2/netlink/generic/__init__.py", line 126, in
> nlm_request
>     return tuple(super().nlm_request(*argv, **kwarg))
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File "[...]/pyroute2/netlink/nlsocket.py", line 1124, in nlm_request
>     self.put(msg, msg_type, msg_flags, msg_seq=msg_seq)
>   File "[...]/pyroute2/netlink/nlsocket.py", line 389, in put
>     self.sendto_gate(msg, addr)
>   File "[...]/pyroute2/netlink/nlsocket.py", line 1056, in sendto_gate
>     msg.encode()
>   File "[...]/pyroute2/netlink/__init__.py", line 1245, in encode
>     offset = self.encode_nlas(offset)
>              ^^^^^^^^^^^^^^^^^^^^^^^^
>   File "[...]/pyroute2/netlink/__init__.py", line 1560, in encode_nlas
>     nla_instance.setvalue(cell[1])
>   File "[...]/pyroute2/netlink/__init__.py", line 1265, in setvalue
>     nlv.setvalue(nla_tuple[1])
>                  ~~~~~~~~~^^^
> IndexError: list index out of range
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

I don't know which pyroute2 version I had used when I tested this
previously, but even on my current system I get this error now.  Thanks
for the fix.

>  tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index b76907ac0092..a2395c3f37a1 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -537,7 +537,7 @@ class ovsactions(nla):
>              for flat_act in parse_flat_map:
>                  if parse_starts_block(actstr, flat_act[0], False):
>                      actstr = actstr[len(flat_act[0]):]
> -                    self["attrs"].append([flat_act[1]])
> +                    self["attrs"].append([flat_act[1], True])
>                      actstr = actstr[strspn(actstr, ", ") :]
>                      parsed = True

Re: [PATCH net-next 2/2] selftests: openvswitch: set value to nla flags

[Adrián Moreno]

On Mon, Jun 03, 2024 at 03:02:46PM GMT, Aaron Conole wrote:
> Adrian Moreno <amorenoz@redhat.com> writes:
>
> > Netlink flags, although they don't have payload at the netlink level,
> > are represented as having a "True" value in pyroute2.
> >
> > Without it, trying to add a flow with a flag-type action (e.g: pop_vlan)
> > fails with the following traceback:
> >
> > Traceback (most recent call last):
> >   File "[...]/ovs-dpctl.py", line 2498, in <module>
> >     sys.exit(main(sys.argv))
> >              ^^^^^^^^^^^^^^
> >   File "[...]/ovs-dpctl.py", line 2487, in main
> >     ovsflow.add_flow(rep["dpifindex"], flow)
> >   File "[...]/ovs-dpctl.py", line 2136, in add_flow
> >     reply = self.nlm_request(
> >             ^^^^^^^^^^^^^^^^^
> >   File "[...]/pyroute2/netlink/nlsocket.py", line 822, in nlm_request
> >     return tuple(self._genlm_request(*argv, **kwarg))
> >                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   File "[...]/pyroute2/netlink/generic/__init__.py", line 126, in
> > nlm_request
> >     return tuple(super().nlm_request(*argv, **kwarg))
> >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   File "[...]/pyroute2/netlink/nlsocket.py", line 1124, in nlm_request
> >     self.put(msg, msg_type, msg_flags, msg_seq=msg_seq)
> >   File "[...]/pyroute2/netlink/nlsocket.py", line 389, in put
> >     self.sendto_gate(msg, addr)
> >   File "[...]/pyroute2/netlink/nlsocket.py", line 1056, in sendto_gate
> >     msg.encode()
> >   File "[...]/pyroute2/netlink/__init__.py", line 1245, in encode
> >     offset = self.encode_nlas(offset)
> >              ^^^^^^^^^^^^^^^^^^^^^^^^
> >   File "[...]/pyroute2/netlink/__init__.py", line 1560, in encode_nlas
> >     nla_instance.setvalue(cell[1])
> >   File "[...]/pyroute2/netlink/__init__.py", line 1265, in setvalue
> >     nlv.setvalue(nla_tuple[1])
> >                  ~~~~~~~~~^^^
> > IndexError: list index out of range
> >
> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > ---
>
> Acked-by: Aaron Conole <aconole@redhat.com>
>
> I don't know which pyroute2 version I had used when I tested this
> previously, but even on my current system I get this error now.  Thanks
> for the fix.
>

Thanks Aaron. I'll resend as v2 with your ack as a stand-alone patch
since the other patch of this series will be fixed by your soon-to-come
series.

> >  tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > index b76907ac0092..a2395c3f37a1 100644
> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > @@ -537,7 +537,7 @@ class ovsactions(nla):
> >              for flat_act in parse_flat_map:
> >                  if parse_starts_block(actstr, flat_act[0], False):
> >                      actstr = actstr[len(flat_act[0]):]
> > -                    self["attrs"].append([flat_act[1]])
> > +                    self["attrs"].append([flat_act[1], True])
> >                      actstr = actstr[strspn(actstr, ", ") :]
> >                      parsed = True
>

Re: [PATCH net-next 2/2] selftests: openvswitch: set value to nla flags

[Aaron Conole]

Adrián Moreno <amorenoz@redhat.com> writes:

> On Mon, Jun 03, 2024 at 03:02:46PM GMT, Aaron Conole wrote:
>> Adrian Moreno <amorenoz@redhat.com> writes:
>>
>> > Netlink flags, although they don't have payload at the netlink level,
>> > are represented as having a "True" value in pyroute2.
>> >
>> > Without it, trying to add a flow with a flag-type action (e.g: pop_vlan)
>> > fails with the following traceback:
>> >
>> > Traceback (most recent call last):
>> >   File "[...]/ovs-dpctl.py", line 2498, in <module>
>> >     sys.exit(main(sys.argv))
>> >              ^^^^^^^^^^^^^^
>> >   File "[...]/ovs-dpctl.py", line 2487, in main
>> >     ovsflow.add_flow(rep["dpifindex"], flow)
>> >   File "[...]/ovs-dpctl.py", line 2136, in add_flow
>> >     reply = self.nlm_request(
>> >             ^^^^^^^^^^^^^^^^^
>> >   File "[...]/pyroute2/netlink/nlsocket.py", line 822, in nlm_request
>> >     return tuple(self._genlm_request(*argv, **kwarg))
>> >                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >   File "[...]/pyroute2/netlink/generic/__init__.py", line 126, in
>> > nlm_request
>> >     return tuple(super().nlm_request(*argv, **kwarg))
>> >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >   File "[...]/pyroute2/netlink/nlsocket.py", line 1124, in nlm_request
>> >     self.put(msg, msg_type, msg_flags, msg_seq=msg_seq)
>> >   File "[...]/pyroute2/netlink/nlsocket.py", line 389, in put
>> >     self.sendto_gate(msg, addr)
>> >   File "[...]/pyroute2/netlink/nlsocket.py", line 1056, in sendto_gate
>> >     msg.encode()
>> >   File "[...]/pyroute2/netlink/__init__.py", line 1245, in encode
>> >     offset = self.encode_nlas(offset)
>> >              ^^^^^^^^^^^^^^^^^^^^^^^^
>> >   File "[...]/pyroute2/netlink/__init__.py", line 1560, in encode_nlas
>> >     nla_instance.setvalue(cell[1])
>> >   File "[...]/pyroute2/netlink/__init__.py", line 1265, in setvalue
>> >     nlv.setvalue(nla_tuple[1])
>> >                  ~~~~~~~~~^^^
>> > IndexError: list index out of range
>> >
>> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> > ---
>>
>> Acked-by: Aaron Conole <aconole@redhat.com>
>>
>> I don't know which pyroute2 version I had used when I tested this
>> previously, but even on my current system I get this error now.  Thanks
>> for the fix.
>>
>
> Thanks Aaron. I'll resend as v2 with your ack as a stand-alone patch
> since the other patch of this series will be fixed by your soon-to-come
> series.

Thanks!

>> >  tools/testing/selftests/net/openvswitch/ovs-dpctl.py | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > index b76907ac0092..a2395c3f37a1 100644
>> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > @@ -537,7 +537,7 @@ class ovsactions(nla):
>> >              for flat_act in parse_flat_map:
>> >                  if parse_starts_block(actstr, flat_act[0], False):
>> >                      actstr = actstr[len(flat_act[0]):]
>> > -                    self["attrs"].append([flat_act[1]])
>> > +                    self["attrs"].append([flat_act[1], True])
>> >                      actstr = actstr[strspn(actstr, ", ") :]
>> >                      parsed = True
>>

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

[Aaron Conole]

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

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

[Jakub Kicinski]

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

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

[Adrián Moreno]

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
>

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

[Aaron Conole]

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