On Mon, Nov 29, 2021 at 8:01 AM Ilya Maximets <[email protected]> wrote: > > On 11/4/21 20:54, Salvatore Daniele wrote: > > Hi Ilya, > > > > Per your feedback on v4, I accounted for igmp_type/code, both in the > > work-around in 1/2, and removing it from match.c entirely in 2/2. > > > > I was not sure the best way to proceed regarding the issue I had > > mentioned on v4 where ofputil_normalize_match__() fails to recognize > > 'igmp/ip,nw_proto=2' as an L3 protocol and normalizes igmp flows, > > removing igmp_type/code. > > > > However, I feel removing the fields as you suggested should solve the > > problem regardless. > > > > Curious to hear your thoughts at your convenience! > > Hi. Thanks for v5 and sorry for it taking so long for me to reply. > > Patches LGTM at the first glance. I'll run a couple of tests with > them and apply if everything will look fine.
Great, thank you Ilya! > > For the matching issue. tp_src field in the OpenFlow structure is > defined as "TCP/UDP/SCTP source port/ICMP type", so it's not intended > to be used for IGMP packets. Matching on IGMP fields also not supported > by the OpenFlow specification. So, it seems correct that OVS rejects > this kind of matching requests. Same for tp_dst. > > Best regards, Ilya Maximets. > > > > > Best, > > > > Salvatore > > > > On Thu, Nov 4, 2021 at 3:39 PM Salvatore Daniele <[email protected]> > > wrote: > >> > >> match_format() prints the keyword "igmp" for flows with the field > >> "ip,nw_proto=2". ofp_parse_protocol does not accept this value, or > >> the values igmp_type or igmp_code. > >> > >> This results in flow dump restoration failing when the ovs-save script > >> is used by "ovs-ctl restart" on a dump of flows containing this match. > >> However, removing the "igmp" keyword entirely could break existing > >> scripts in stable branches. > >> > >> The first patch addresses this issue by providing a workaround within > >> ovs-save to preserve the 'igmp' keywords while allowing flows to be > >> restored. This change would be backported to all stable branches. > >> > >> The second patch removes the 'igmp' outputs entirely, replacing it with > >> 'ip,nw_proto=2'. This has been added to NEWS, and would be applied > >> in master branch only. > >> > >> While it might make sense to eventually remove the ovs-save workaround, > >> it remains in this second patch to ensure flows can be restored when > >> upgrading ovs-vswitchd from older versions. > >> > >> v5 > >> - Handle igmp_type/code in workaround and remove them from match.c in > >> 2/2 > >> v4 > >> - Include the ovs-save workaround in both patch 1/2 and 2/2 to address > >> upgrade case discussed in v3 > >> v3 > >> - Fixed typos to be inline with DCO policy > >> - Rebased ontop of latest master > >> v2 > >> - Address comments made on v1 with regard to a work around for stable > >> branches > >> > >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1982743 > >> > >> Signed-off-by: Salvatore Daniele <[email protected]> > >> Signed-off-by: Adrian Moreno <[email protected]> > >> Co-authored-by: Adrian Moreno <[email protected]> > >> > >> Adrian Moreno (1): > >> Match: Do not print "igmp" match keyword > >> > >> Salvatore Daniele (1): > >> ovs-save: Save igmp flows in ofp_parse syntax > >> > >> NEWS | 2 ++ > >> lib/match.c | 6 ------ > >> tests/ovs-ofctl.at | 6 ++++++ > >> utilities/ovs-save | 5 ++++- > >> 4 files changed, 12 insertions(+), 7 deletions(-) > >> > >> -- > >> 2.31.1 > >> > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
