| Message ID | 20240529070320.399974-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev] northd: Fix the match not being cleared inside the loop. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 5/29/24 09:03, Ales Musil wrote: > The match wasn't cleared which led to matches being appended together > and the ovn-controller failed to parse them. > > Fixes: 3faadc76ad71 ("northd: Fix pmtud for non routed traffic.") > Signed-off-by: Ales Musil <amusil@redhat.com> > --- Thanks, Ales, for the fix, it looks correct to me. Can we also add a small unit test in ovn-northd.at? Regards, Dumitru > northd/northd.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 495b838fc..a78cbcd53 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -11989,9 +11989,9 @@ build_lswitch_icmp_packet_toobig_admin_flows( > { > ovs_assert(op->nbsp); > > - ds_clear(match); > if (!lsp_is_router(op->nbsp)) { > for (size_t i = 0; i < op->n_lsp_addrs; i++) { > + ds_clear(match); > ds_put_format(match, > "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||" > " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&" > @@ -12011,6 +12011,7 @@ build_lswitch_icmp_packet_toobig_admin_flows( > return; > } > > + ds_clear(match); > if (peer->od->is_gw_router) { > ds_put_format(match, > "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
On Wed, May 29, 2024 at 9:31 AM Dumitru Ceara <dceara@redhat.com> wrote: > On 5/29/24 09:03, Ales Musil wrote: > > The match wasn't cleared which led to matches being appended together > > and the ovn-controller failed to parse them. > > > > Fixes: 3faadc76ad71 ("northd: Fix pmtud for non routed traffic.") > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > Thanks, Ales, for the fix, it looks correct to me. > > Can we also add a small unit test in ovn-northd.at? > I have updated one of the existing tests to include this scenario in v2. > > Regards, > Dumitru > > > northd/northd.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 495b838fc..a78cbcd53 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -11989,9 +11989,9 @@ build_lswitch_icmp_packet_toobig_admin_flows( > > { > > ovs_assert(op->nbsp); > > > > - ds_clear(match); > > if (!lsp_is_router(op->nbsp)) { > > for (size_t i = 0; i < op->n_lsp_addrs; i++) { > > + ds_clear(match); > > ds_put_format(match, > > "((ip4 && icmp4.type == 3 && icmp4.code == 4) > ||" > > " (ip6 && icmp6.type == 2 && icmp6.code == > 0)) &&" > > @@ -12011,6 +12011,7 @@ build_lswitch_icmp_packet_toobig_admin_flows( > > return; > > } > > > > + ds_clear(match); > > if (peer->od->is_gw_router) { > > ds_put_format(match, > > "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||" > > Thanks, Ales
diff --git a/northd/northd.c b/northd/northd.c index 495b838fc..a78cbcd53 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -11989,9 +11989,9 @@ build_lswitch_icmp_packet_toobig_admin_flows( { ovs_assert(op->nbsp); - ds_clear(match); if (!lsp_is_router(op->nbsp)) { for (size_t i = 0; i < op->n_lsp_addrs; i++) { + ds_clear(match); ds_put_format(match, "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||" " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&" @@ -12011,6 +12011,7 @@ build_lswitch_icmp_packet_toobig_admin_flows( return; } + ds_clear(match); if (peer->od->is_gw_router) { ds_put_format(match, "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
The match wasn't cleared which led to matches being appended together and the ovn-controller failed to parse them. Fixes: 3faadc76ad71 ("northd: Fix pmtud for non routed traffic.") Signed-off-by: Ales Musil <amusil@redhat.com> --- northd/northd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)