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

Aaron Conole
2024-06-17T17:07:26Z

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