Received-SPF: pass (google.com: domain of linux-kernel+bounces-199592-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Aaron Conole <aconole@redhat.com>
To: Adrian Moreno <amorenoz@redhat.com>
Cc: netdev@vger.kernel.org,  Pravin B Shelar <pshelar@ovn.org>,  "David S.
 Miller" <davem@davemloft.net>,  Eric Dumazet <edumazet@google.com>,  Jakub
 Kicinski <kuba@kernel.org>,  Paolo Abeni <pabeni@redhat.com>,  Shuah Khan
 <shuah@kernel.org>,  dev@openvswitch.org,
  linux-kselftest@vger.kernel.org,  linux-kernel@vger.kernel.org, Simon
 Horman <horms@verge.net.au>
Subject: Re: [PATCH net-next 1/2] selftests: openvswitch: fix action formatting
In-Reply-To: <20240603183121.2305013-1-amorenoz@redhat.com> (Adrian Moreno's
	message of "Mon, 3 Jun 2024 20:31:19 +0200")
References: <20240603183121.2305013-1-amorenoz@redhat.com>
Date: Mon, 03 Jun 2024 15:00:03 -0400
Message-ID: <f7ta5k126oc.fsf@redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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