[ovs-dev] [PATCH v4 1/2] ovs-save: Save igmp flows in ofp_parse syntax

[Salvatore Daniele]

Good point, OVS is unable to parse them. I don't see anywhere in the OVS
code that relies on these fields being printed.

I could replace them with the default 'tp_src/dst' in this workaround, so
as not to break any scripts.

That being said, I am not sure how these fields would currently even end up
in a flow-dump so perhaps that would be redundant. igmp* fields can not be
added explicitly (add-flow ... igmp_type=1) the way you could any other l3
field/type since they can't be parsed.

Additionally, it would seem ofputil_normalize_match__() does not recognize
'igmp/ip,nw_proto=2' as an L3 protocol.
Setting "tp_src" should work to insert a flow with 'igmp_type', as tp_src
is changed to igmp_type internally, but a flow cannot match on an L3 field
without saying what L3 protocol is in use, and since 'igmp' is not
recognized as such, the flow is normalized removing the igmp_type field.

$ ovs-ofctl add-flow br1 "ip,nw_proto=2,tp_src=1, action=drop"
2021-08-05T15:16:48Z|00001|ofp_match|INFO|normalization changed ofp_match,
details:
2021-08-05T15:16:48Z|00002|ofp_match|INFO| pre: igmp,igmp_type=1
2021-08-05T15:16:48Z|00003|ofp_match|INFO|post: igmp

I could fix this issue in ofputil_normalize_match__() in this patch so igmp
packets will behave like any other L3 proto, which would then require me to
update the workaround. However, if the goal of this first patch is to avoid
breaking existing scripts, perhaps it would make sense to leave the
ofputil_normalize_match__() as is?

I could then fix the normalization behavior in the second patch in addition
to removing 'igmp' and 'igmp fields' from match.c.

What do you think?

On Wed, Aug 4, 2021 at 1:51 PM Ilya Maximets <i.maximets at ovn.org> wrote:

> On 7/23/21 4:58 PM, Salvatore Daniele wrote:
> > match.c generates the keyword "igmp", which is not supported in
> ofp-parse.
> > This means that flow dumps containing 'igmp' can not be restored.
> >
> > Removing the 'igmp' keyword entirely could break existing scripts in
> stable
> > branches, so this patch creates a workaround within ovs-save by
> converting any
> > instances of "igmp" within $bridge.flows.dump into "ip, nw_proto=2".
> >
> > Signed-off-by: Salvatore Daniele <sdaniele at redhat.com>
> > Acked-by: Aaron Conole <aconole at redhat.com>
> > ---
> >  utilities/ovs-save | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/utilities/ovs-save b/utilities/ovs-save
> > index 27ce3a9aa..23cb0d9d9 100755
> > --- a/utilities/ovs-save
> > +++ b/utilities/ovs-save
> > @@ -150,7 +150,8 @@ save_flows () {
> >          ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats
> "$bridge" | \
> >              sed -e '/NXST_FLOW/d' \
> >                  -e '/OFPST_FLOW/d' \
> > -                -e 's/\(idle\|hard\)_age=[^,]*,//g' > \
> > +                -e 's/\(idle\|hard\)_age=[^,]*,//g' \
> > +                -e 's/igmp/ip,nw_proto=2/g' > \
> >                  "$workdir/$bridge.flows.dump"
> >      done
> >      echo "rm -rf \"$workdir\""
> >
>
> Hmm.  What about 'igmp_type' and 'igmp_code'?  I don't think that OVS
> will be able to parse them.  Should we replace them with generic
> 'tp_src' and 'tp_dst' ?  Will that work?
>
> Best regards, Ilya Maximets.
>
>