Re: [ovs-dev] [PATCH 1/1] match: do not print "igmp" match keyword
On 7/7/21 8:41 PM, Ilya Maximets wrote: > On 7/6/21 3:36 PM, Flavio Leitner wrote: >> On Tue, Jul 06, 2021 at 03:27:41PM +0200, Adrian Moreno wrote: >>> >>> >>> On 7/6/21 2:50 PM, Flavio Leitner wrote: On Tue, Jul 06, 2021 at 08:25:59AM +0200, Adrian Moreno wrote: > > > On 7/5/21 4:15 PM, Flavio Leitner wrote: >> >> Hi, >> >> On Wed, Jun 30, 2021 at 05:43:54PM +0200, Adrian Moreno wrote: >>> The match keyword "igmp" is not supported in ofp-parse, which means >>> that flow dumps cannot be restored. This patch prints the igmp match >>> in the accepted format (ip,nw_proto=2) and adds a test. >> >> I raised concerns about changing the output and break scripts in >> the past. However, it seems not removing the keyword also cause >> issues, so I am not opposing to remove the igmp keyword anymore. >> >> Acked-by: Flavio Leitner >> > > Thanks Flavio. Do you think this is an acceptable solution also for > stable branches? My concern is that changing the output can potentially break somebody else's script and that is really bad in a stable release update. BTW, this is an user visible change, so I'd say that the patch needs to highlight that in the NEWS file too. >>> OK. I'll send another update, thanks. >>> > If not, how about replacing the flows in ovs-save so that upgrades of > stable > branches work fine? You mean fixing ovs-save in master or in stable branches? >>> My proposal was: >>> - changing the output + advertise in NEWS in master branch (and future >>> releases) >>> - add a workaround in ovs-save in stable branches to ensure they can be >>> upgraded >>> without big datapath impact >>> >>> WDYT? >> >> Sounds like a good plan to me. > > Sounds good to me too. This way we will change the behavior in current > release and will fix the existing issue in ovs-save on stable branches. > > Adrian, could you send a v2 as a patch set where the first patch implements > a workaround in ovs-save (this one we will apply to master and backport) > and the second patch changes the actual output (and removes the workaround > from ovs-save?) ? > Will do, thanks Ilya > Best regards, Ilya Maximets. > -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] match: do not print "igmp" match keyword
On 7/6/21 3:36 PM, Flavio Leitner wrote: > On Tue, Jul 06, 2021 at 03:27:41PM +0200, Adrian Moreno wrote: >> >> >> On 7/6/21 2:50 PM, Flavio Leitner wrote: >>> On Tue, Jul 06, 2021 at 08:25:59AM +0200, Adrian Moreno wrote: On 7/5/21 4:15 PM, Flavio Leitner wrote: > > Hi, > > On Wed, Jun 30, 2021 at 05:43:54PM +0200, Adrian Moreno wrote: >> The match keyword "igmp" is not supported in ofp-parse, which means >> that flow dumps cannot be restored. This patch prints the igmp match >> in the accepted format (ip,nw_proto=2) and adds a test. > > I raised concerns about changing the output and break scripts in > the past. However, it seems not removing the keyword also cause > issues, so I am not opposing to remove the igmp keyword anymore. > > Acked-by: Flavio Leitner > Thanks Flavio. Do you think this is an acceptable solution also for stable branches? >>> >>> My concern is that changing the output can potentially break >>> somebody else's script and that is really bad in a stable >>> release update. >>> >>> BTW, this is an user visible change, so I'd say that the patch >>> needs to highlight that in the NEWS file too. >>> >> OK. I'll send another update, thanks. >> >>> If not, how about replacing the flows in ovs-save so that upgrades of stable branches work fine? >>> >>> You mean fixing ovs-save in master or in stable branches? >>> >> My proposal was: >> - changing the output + advertise in NEWS in master branch (and future >> releases) >> - add a workaround in ovs-save in stable branches to ensure they can be >> upgraded >> without big datapath impact >> >> WDYT? > > Sounds like a good plan to me. Sounds good to me too. This way we will change the behavior in current release and will fix the existing issue in ovs-save on stable branches. Adrian, could you send a v2 as a patch set where the first patch implements a workaround in ovs-save (this one we will apply to master and backport) and the second patch changes the actual output (and removes the workaround from ovs-save?) ? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] match: do not print "igmp" match keyword
On Tue, Jul 06, 2021 at 03:27:41PM +0200, Adrian Moreno wrote: > > > On 7/6/21 2:50 PM, Flavio Leitner wrote: > > On Tue, Jul 06, 2021 at 08:25:59AM +0200, Adrian Moreno wrote: > >> > >> > >> On 7/5/21 4:15 PM, Flavio Leitner wrote: > >>> > >>> Hi, > >>> > >>> On Wed, Jun 30, 2021 at 05:43:54PM +0200, Adrian Moreno wrote: > The match keyword "igmp" is not supported in ofp-parse, which means > that flow dumps cannot be restored. This patch prints the igmp match > in the accepted format (ip,nw_proto=2) and adds a test. > >>> > >>> I raised concerns about changing the output and break scripts in > >>> the past. However, it seems not removing the keyword also cause > >>> issues, so I am not opposing to remove the igmp keyword anymore. > >>> > >>> Acked-by: Flavio Leitner > >>> > >> > >> Thanks Flavio. Do you think this is an acceptable solution also for stable > >> branches? > > > > My concern is that changing the output can potentially break > > somebody else's script and that is really bad in a stable > > release update. > > > > BTW, this is an user visible change, so I'd say that the patch > > needs to highlight that in the NEWS file too. > > > OK. I'll send another update, thanks. > > > > >> If not, how about replacing the flows in ovs-save so that upgrades of > >> stable > >> branches work fine? > > > > You mean fixing ovs-save in master or in stable branches? > > > My proposal was: > - changing the output + advertise in NEWS in master branch (and future > releases) > - add a workaround in ovs-save in stable branches to ensure they can be > upgraded > without big datapath impact > > WDYT? Sounds like a good plan to me. Thank you, -- fbl ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] match: do not print "igmp" match keyword
On 7/6/21 2:50 PM, Flavio Leitner wrote: > On Tue, Jul 06, 2021 at 08:25:59AM +0200, Adrian Moreno wrote: >> >> >> On 7/5/21 4:15 PM, Flavio Leitner wrote: >>> >>> Hi, >>> >>> On Wed, Jun 30, 2021 at 05:43:54PM +0200, Adrian Moreno wrote: The match keyword "igmp" is not supported in ofp-parse, which means that flow dumps cannot be restored. This patch prints the igmp match in the accepted format (ip,nw_proto=2) and adds a test. >>> >>> I raised concerns about changing the output and break scripts in >>> the past. However, it seems not removing the keyword also cause >>> issues, so I am not opposing to remove the igmp keyword anymore. >>> >>> Acked-by: Flavio Leitner >>> >> >> Thanks Flavio. Do you think this is an acceptable solution also for stable >> branches? > > My concern is that changing the output can potentially break > somebody else's script and that is really bad in a stable > release update. > > BTW, this is an user visible change, so I'd say that the patch > needs to highlight that in the NEWS file too. > OK. I'll send another update, thanks. > >> If not, how about replacing the flows in ovs-save so that upgrades of stable >> branches work fine? > > You mean fixing ovs-save in master or in stable branches? > My proposal was: - changing the output + advertise in NEWS in master branch (and future releases) - add a workaround in ovs-save in stable branches to ensure they can be upgraded without big datapath impact WDYT? -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] match: do not print "igmp" match keyword
On Tue, Jul 06, 2021 at 08:25:59AM +0200, Adrian Moreno wrote: > > > On 7/5/21 4:15 PM, Flavio Leitner wrote: > > > > Hi, > > > > On Wed, Jun 30, 2021 at 05:43:54PM +0200, Adrian Moreno wrote: > >> The match keyword "igmp" is not supported in ofp-parse, which means > >> that flow dumps cannot be restored. This patch prints the igmp match > >> in the accepted format (ip,nw_proto=2) and adds a test. > > > > I raised concerns about changing the output and break scripts in > > the past. However, it seems not removing the keyword also cause > > issues, so I am not opposing to remove the igmp keyword anymore. > > > > Acked-by: Flavio Leitner > > > > Thanks Flavio. Do you think this is an acceptable solution also for stable > branches? My concern is that changing the output can potentially break somebody else's script and that is really bad in a stable release update. BTW, this is an user visible change, so I'd say that the patch needs to highlight that in the NEWS file too. > If not, how about replacing the flows in ovs-save so that upgrades of stable > branches work fine? You mean fixing ovs-save in master or in stable branches? -- fbl ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] match: do not print "igmp" match keyword
On 7/5/21 4:15 PM, Flavio Leitner wrote: > > Hi, > > On Wed, Jun 30, 2021 at 05:43:54PM +0200, Adrian Moreno wrote: >> The match keyword "igmp" is not supported in ofp-parse, which means >> that flow dumps cannot be restored. This patch prints the igmp match >> in the accepted format (ip,nw_proto=2) and adds a test. > > I raised concerns about changing the output and break scripts in > the past. However, it seems not removing the keyword also cause > issues, so I am not opposing to remove the igmp keyword anymore. > > Acked-by: Flavio Leitner > Thanks Flavio. Do you think this is an acceptable solution also for stable branches? If not, how about replacing the flows in ovs-save so that upgrades of stable branches work fine? Thanks. -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] match: do not print "igmp" match keyword
Hi, On Wed, Jun 30, 2021 at 05:43:54PM +0200, Adrian Moreno wrote: > The match keyword "igmp" is not supported in ofp-parse, which means > that flow dumps cannot be restored. This patch prints the igmp match > in the accepted format (ip,nw_proto=2) and adds a test. I raised concerns about changing the output and break scripts in the past. However, it seems not removing the keyword also cause issues, so I am not opposing to remove the igmp keyword anymore. Acked-by: Flavio Leitner Thanks, fbl ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/1] match: do not print "igmp" match keyword
The match keyword "igmp" is not supported in ofp-parse, which means
that flow dumps cannot be restored. This patch prints the igmp match
in the accepted format (ip,nw_proto=2) and adds a test.
Signed-off-by: Adrian Moreno
---
lib/match.c| 2 --
tests/ovs-ofctl.at | 2 ++
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/match.c b/lib/match.c
index ba716579d..4a0778c30 100644
--- a/lib/match.c
+++ b/lib/match.c
@@ -1556,8 +1556,6 @@ match_format(const struct match *match,
skip_proto = true;
if (f->nw_proto == IPPROTO_ICMP) {
ds_put_format(s, "%sicmp%s,", colors.value, colors.end);
-} else if (f->nw_proto == IPPROTO_IGMP) {
-ds_put_format(s, "%sigmp%s,", colors.value, colors.end);
} else if (f->nw_proto == IPPROTO_TCP) {
ds_put_format(s, "%stcp%s,", colors.value, colors.end);
} else if (f->nw_proto == IPPROTO_UDP) {
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 5ddca67e7..fbc622959 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -192,6 +192,7 @@ actions=note:41.42.43,note:00.01.02.03.04.05.06.07,note
ip,actions=set_field:10.4.3.77->ip_src,mod_nw_ecn:2
sctp actions=drop
sctp actions=drop
+ip,nw_proto=2 actions=drop
in_port=0 actions=resubmit:0
actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678,ingress)
@@ -226,6 +227,7 @@ OFPT_FLOW_MOD: ADD
actions=note:41.42.43.00.00.00,note:00.01.02.03.04.05.06.07.0
OFPT_FLOW_MOD: ADD ip actions=mod_nw_src:10.4.3.77,load:0x2->NXM_NX_IP_ECN[]
OFPT_FLOW_MOD: ADD sctp actions=drop
OFPT_FLOW_MOD: ADD sctp actions=drop
+OFPT_FLOW_MOD: ADD ip,nw_proto=2 actions=drop
OFPT_FLOW_MOD: ADD in_port=0 actions=resubmit:0
OFPT_FLOW_MOD: ADD
actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
OFPT_FLOW_MOD: ADD
actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678,ingress)
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
