Hi Adrian, On Sat, Apr 1, 2023 at 12:39 AM Adrian Moreno <[email protected]> wrote: > > Hi Wan, > > Thanks for submitting this patch. > > On 11/18/22 09:13, Wan Junjie wrote: > > According to the ovs-fields manpage, 'TLVs must be ordered so that > > a field’s prerequisites are satisfied before if is parsed'. > > > I believe this sentence is talking about the binary Openflow (>1.2) format > where > match fields are encoded in TLV. > > The way we represent flow matches in human-readable strings (for ovs-ofctl) is > more flexible, allowing the user to express the matches in any arbitrary > order. > For example, we could add a flow such as: > "tcp_src=255,ip_src=1.1.1.1,ip,eth_src=ca:fe:ca:fe:ca:fe,tcp,actions=drop" > > which is clearly in a very weird order, but if we looked at the wire we'd see > the TLV sections of the OFP packet are in the right order. > > > However, a match pattern like 'tcp,ipv6' can be parsed as 'tcp6', > > meanwhile 'ipv6,tcp' is parsed as 'tcp'. So a limitation here make > > sure that the order should be respected. > > > > I understand this might be a bit confusing. > Firstly, according to the ovs-fields(7) these are the meanings of the > shorthands: > > ip: eth_type=0x0800, > ipv6: eth_type=0x86dd > tcp: eth_type=0x0800,ip_proto=6 > tcp6: eth_type=0x86dd,ip_proto=6 > > If we expand the shorthands: > > "tcp,ipv6" == "eth_type=0x0800,ip_proto=6,eth_type=0x86dd" > "ipv6,tcp" == "eth_type=0x86dd,eth_type=0x0800,ip_proto=6" > > So the last "eth_type" match prevails.
Thanks for your clarification. I saw the shorthands. IMHO flexibility input should not have any side effect over correctness. At least we should print a reminder to the user. Or add some documents instead? Regards, Wan Junjie > > > Signed-off-by: Wan Junjie <[email protected]> > > --- > > lib/ofp-flow.c | 3 +++ > > tests/ofproto-dpif.at | 8 ++++---- > > tests/ovs-ofctl.at | 15 +++++++++++++++ > > 3 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c > > index 3bc744f78..db9b75c7b 100644 > > --- a/lib/ofp-flow.c > > +++ b/lib/ofp-flow.c > > @@ -1580,6 +1580,9 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int > > command, char *string, > > char *error = NULL; > > > > if (ofp_parse_protocol(name, &p)) { > > + if (match.flow.nw_proto) { > > + return xstrdup("TLVs must be ordered"); > > + } > > match_set_dl_type(&match, htons(p->dl_type)); > > if (p->nw_proto) { > > match_set_nw_proto(&match, p->nw_proto); > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > > index 728243183..a4a7ab5fb 100644 > > --- a/tests/ofproto-dpif.at > > +++ b/tests/ofproto-dpif.at > > @@ -11629,14 +11629,14 @@ table=1,priority=1,action=drop > > dnl > > dnl Table 2 > > dnl > > -table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,tcp,action=ct(commit,zone=1),ct(table=3,zone=2) > > -table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,tcp,action=ct(table=3,zone=2) > > +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+new,action=ct(commit,zone=1),ct(table=3,zone=2) > > +table=2,priority=10,in_port=1,tcp,ct_zone=1,ct_state=+trk+est,action=ct(table=3,zone=2) > > table=2,priority=1,action=drop > > dnl > > dnl Table 3 > > dnl > > -table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,tcp,action=ct(commit,zone=2),4 > > -table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,tcp,action=3 > > +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+new,action=ct(commit,zone=2),4 > > +table=3,priority=10,in_port=1,tcp,ct_zone=2,ct_state=+trk+est,action=3 > > table=2,priority=1,action=drop > > ]) > > > > diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at > > index a8934051e..b933924b8 100644 > > --- a/tests/ovs-ofctl.at > > +++ b/tests/ovs-ofctl.at > > @@ -381,6 +381,21 @@ do > > done > > AT_CLEANUP > > > > +AT_SETUP([ovs-ofctl parse-flow with protocol order]) > > +for test_case in \ > > + 'tcp,ip' \ > > + 'tcp,ipv6' \ > > + 'udp,ip' \ > > + 'udp,ip6' > > +do > > + set $test_case > > + field=$1 > > + AT_CHECK_UNQUOTED([ovs-ofctl parse-flow "$field,actions=drop"], [1], > > [], > > +[ovs-ofctl: TLVs must be ordered > > +]) > > +done > > +AT_CLEANUP > > + > > AT_SETUP([ovs-ofctl action inconsistency (OpenFlow 1.1)]) > > OVS_VSWITCHD_START > > add_of_ports br0 1 2 3 > > -- > Adrián Moreno > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
