| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f7tsexjpodd.fsf@redhat.com>
Date: Tue, 11 Jun 2024 10:04:14 -0400
From: Aaron Conole <aconole@...hat.com>
To: Adrián Moreno <amorenoz@...hat.com>
Cc: netdev@...r.kernel.org, Pravin B Shelar <pshelar@....org>, "David S.
Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Shuah Khan
<shuah@...nel.org>, dev@...nvswitch.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org, Simon
Horman <horms@...ge.net.au>
Subject: Re: [PATCH net-next 1/2] selftests: openvswitch: fix action formatting
Adrián Moreno <amorenoz@...hat.com> writes:
> On Mon, Jun 03, 2024 at 03:00:03PM GMT, Aaron Conole wrote:
>> Adrian Moreno <amorenoz@...hat.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@...hat.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
>>
Powered by blists - more mailing lists