| Message ID | 20210224232649.1227870-1-blp@ovn.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ovs-dev] ovn-northd: Warn for >1 IPv4 in build_lrouter_force_snat_flows_op(). | expand |
On Thu, Feb 25, 2021 at 4:57 AM Ben Pfaff <blp@ovn.org> wrote: > > The comments here point out that more than one IPv4 address is a > problem (versus more than 2 IPv6 addresses), but the code doesn't match. > Oops. My mistake. Thanks for catching this. Looks like you have addressed fixing the logic in the ddlog patch itself and this patch is not required :). Thanks Numan > I didn't test this, it's just an observation from reading code and > comments. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > CC: Numan Siddique <numans@ovn.org> > Fixes: c6e21a23bd8c ("northd: Provide the Gateway router option 'lb_force_snat_ip' to take router port ips.") > --- > northd/ovn-northd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 85022d36c6ae..ac872aadea3d 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -9164,7 +9164,7 @@ build_lrouter_force_snat_flows_op(struct ovn_port *op, > op->lrp_networks.ipv4_addrs[0].addr_s); > ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 110, > ds_cstr(match), ds_cstr(actions)); > - if (op->lrp_networks.n_ipv4_addrs > 2) { > + if (op->lrp_networks.n_ipv4_addrs > 1) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > VLOG_WARN_RL(&rl, "Logical router port %s is configured with " > "multiple IPv4 addresses. Only the first " > -- > 2.29.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 25/02/2021 17:51, Numan Siddique wrote: > On Thu, Feb 25, 2021 at 4:57 AM Ben Pfaff <blp@ovn.org> wrote: >> >> The comments here point out that more than one IPv4 address is a >> problem (versus more than 2 IPv6 addresses), but the code doesn't match. >> > > Oops. My mistake. Thanks for catching this. And mine! I reviewed it > > Looks like you have addressed fixing the logic in the ddlog patch > itself and this patch is not > required :). > > Thanks > Numan > >> I didn't test this, it's just an observation from reading code and >> comments. >> >> Signed-off-by: Ben Pfaff <blp@ovn.org> >> CC: Numan Siddique <numans@ovn.org> >> Fixes: c6e21a23bd8c ("northd: Provide the Gateway router option 'lb_force_snat_ip' to take router port ips.") >> --- >> northd/ovn-northd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index 85022d36c6ae..ac872aadea3d 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -9164,7 +9164,7 @@ build_lrouter_force_snat_flows_op(struct ovn_port *op, >> op->lrp_networks.ipv4_addrs[0].addr_s); >> ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 110, >> ds_cstr(match), ds_cstr(actions)); >> - if (op->lrp_networks.n_ipv4_addrs > 2) { >> + if (op->lrp_networks.n_ipv4_addrs > 1) { >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> VLOG_WARN_RL(&rl, "Logical router port %s is configured with " >> "multiple IPv4 addresses. Only the first " >> -- >> 2.29.2 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Feb 25, 2021 at 11:21:26PM +0530, Numan Siddique wrote: > On Thu, Feb 25, 2021 at 4:57 AM Ben Pfaff <blp@ovn.org> wrote: > > > > The comments here point out that more than one IPv4 address is a > > problem (versus more than 2 IPv6 addresses), but the code doesn't match. > > > > Oops. My mistake. Thanks for catching this. > > Looks like you have addressed fixing the logic in the ddlog patch > itself and this patch is not > required :). Oh, I apologize. That should not have been folded into my ddlog patch since it's a separate issue. I made a mistake. Thanks for verifying that it's correct!
On Thu, Feb 25, 2021 at 11:43 PM Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Feb 25, 2021 at 11:21:26PM +0530, Numan Siddique wrote: > > On Thu, Feb 25, 2021 at 4:57 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > The comments here point out that more than one IPv4 address is a > > > problem (versus more than 2 IPv6 addresses), but the code doesn't match. > > > > > > > Oops. My mistake. Thanks for catching this. > > > > Looks like you have addressed fixing the logic in the ddlog patch > > itself and this patch is not > > required :). > > Oh, I apologize. That should not have been folded into my ddlog patch > since it's a separate issue. I made a mistake. No worries. Thanks Numan > > Thanks for verifying that it's correct! > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 85022d36c6ae..ac872aadea3d 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -9164,7 +9164,7 @@ build_lrouter_force_snat_flows_op(struct ovn_port *op, op->lrp_networks.ipv4_addrs[0].addr_s); ovn_lflow_add(lflows, op->od, S_ROUTER_OUT_SNAT, 110, ds_cstr(match), ds_cstr(actions)); - if (op->lrp_networks.n_ipv4_addrs > 2) { + if (op->lrp_networks.n_ipv4_addrs > 1) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); VLOG_WARN_RL(&rl, "Logical router port %s is configured with " "multiple IPv4 addresses. Only the first "
The comments here point out that more than one IPv4 address is a problem (versus more than 2 IPv6 addresses), but the code doesn't match. I didn't test this, it's just an observation from reading code and comments. Signed-off-by: Ben Pfaff <blp@ovn.org> CC: Numan Siddique <numans@ovn.org> Fixes: c6e21a23bd8c ("northd: Provide the Gateway router option 'lb_force_snat_ip' to take router port ips.") --- northd/ovn-northd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)