Re: [RFC net-next 3/7] selftests: openvswitch: Add set() and set_masked() support.

[Aaron Conole <aconole@redhat.com>]

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

> On Thu, Jun 13, 2024 at 02:13:29PM GMT, Aaron Conole wrote:
>> These will be used in upcoming commits to set specific attributes for
>> interacting with tunnels.  Since set() will use the key parsing routine, we
>> also make sure to prepend it with an open paren, for the action parsing to
>> properly understand it.
>>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  .../selftests/net/openvswitch/ovs-dpctl.py    | 39 +++++++++++++++++--
>>  1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> index 73768f3af6e5..fee64c31d4d4 100644
>> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> @@ -284,7 +284,7 @@ class ovsactions(nla):
>>          ("OVS_ACTION_ATTR_UNSPEC", "none"),
>>          ("OVS_ACTION_ATTR_OUTPUT", "uint32"),
>>          ("OVS_ACTION_ATTR_USERSPACE", "userspace"),
>> -        ("OVS_ACTION_ATTR_SET", "none"),
>> +        ("OVS_ACTION_ATTR_SET", "ovskey"),
>>          ("OVS_ACTION_ATTR_PUSH_VLAN", "none"),
>>          ("OVS_ACTION_ATTR_POP_VLAN", "flag"),
>>          ("OVS_ACTION_ATTR_SAMPLE", "none"),
>> @@ -292,7 +292,7 @@ class ovsactions(nla):
>>          ("OVS_ACTION_ATTR_HASH", "none"),
>>          ("OVS_ACTION_ATTR_PUSH_MPLS", "none"),
>>          ("OVS_ACTION_ATTR_POP_MPLS", "flag"),
>> -        ("OVS_ACTION_ATTR_SET_MASKED", "none"),
>> +        ("OVS_ACTION_ATTR_SET_MASKED", "ovskey"),
>>          ("OVS_ACTION_ATTR_CT", "ctact"),
>>          ("OVS_ACTION_ATTR_TRUNC", "uint32"),
>>          ("OVS_ACTION_ATTR_PUSH_ETH", "none"),
>> @@ -469,6 +469,14 @@ class ovsactions(nla):
>>                      print_str += "clone("
>>                      print_str += datum.dpstr(more)
>>                      print_str += ")"
>> +                elif field[0] == "OVS_ACTION_ATTR_SET" or \
>> +                     field[0] == "OVS_ACTION_ATTR_SET_MASKED":
>> +                    print_str += "set"
>> +                    if field[0] == "OVS_ACTION_ATTR_SET_MASKED":
>> +                        print_str += "_masked"
>> +                    print_str += "("
>> +                    print_str += datum.dpstr(more)
>> +                    print_str += ")"
>>                  else:
>>                      try:
>>                          print_str += datum.dpstr(more)
>> @@ -547,6 +555,25 @@ class ovsactions(nla):
>>                  self["attrs"].append(("OVS_ACTION_ATTR_CLONE", subacts))
>>                  actstr = actstr[parsedLen:]
>>                  parsed = True
>> +            elif parse_starts_block(actstr, "set(", False):
>> +                parencount += 1
>> +                k = ovskey()
>> +                actstr = actstr[len("set("):]
>> +                actstr = k.parse(actstr, None)
>> +                self["attrs"].append(("OVS_ACTION_ATTR_SET", k))
>> +                if not actstr.startswith(")"):
>> +                    actstr = ")" + actstr
>> +                parsed = True
>> +            elif parse_starts_block(actstr, "set_masked(", False):
>> +                parencount += 1
>> +                k = ovskey()
>> +                m = ovskey()
>> +                actstr = actstr[len("set_masked("):]
>> +                actstr = k.parse(actstr, m)
>> +                self["attrs"].append(("OVS_ACTION_ATTR_SET_MASKED", [k, m]))
>> +                if not actstr.startswith(")"):
>> +                    actstr = ")" + actstr
>> +                parsed = True
>>              elif parse_starts_block(actstr, "ct(", False):
>>                  parencount += 1
>>                  actstr = actstr[len("ct(") :]
>> @@ -1312,7 +1339,7 @@ class ovskey(nla):
>>                  mask["attrs"].append([field[0], m])
>>              self["attrs"].append([field[0], k])
>>
>> -            flowstr = flowstr[strspn(flowstr, "),") :]
>> +            flowstr = flowstr[strspn(flowstr, "), ") :]
>>
>>          return flowstr
>>
>> @@ -1898,7 +1925,11 @@ class OvsFlow(GenericNetlinkSocket):
>>              ):
>>                  print_str += "drop"
>>              else:
>> -                print_str += actsmsg.dpstr(more)
>> +                if type(actsmsg) == "list":
>
> nit: I belive the recommended way of comparing types is using
> "isinstance":
>
> https://www.flake8rules.com/rules/E721.html
>
> Also, I don't see what can make actmsg be a list. It should always be an
> instance of "ovsactions", right?

Yes, you're right.  This was some debug code that I was messing with and
it made it into this submission.  I've dropped it :)  Thanks for the review!

>
>> +                    for act in actsmsg:
>> +                        print_str += act.dpstr(more)
>> +                else:
>> +                    print_str += actsmsg.dpstr(more)
>>
>>              return print_str
>>
>> --
>> 2.45.1
>>