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/7262107de7170d44b6dbf6c5e...
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