On 1/5/26 11:02 AM, Ales Musil via dev wrote: > The nexthop of Logical Router Policy can be NULL or empty, make sure > the northd doesn't crash when that happens:
Hi, Ales. Not a full review, but see a few comments below. The patch subject, IMO, is too generic and can't be used to find the issue in the git history. Should be something like "northd: Fix crash on explicit output-port reroute without nexthop". Or something like this. > > northd/northd.c:14771:39: runtime error: null pointer passed as argument 1, > which is declared to never be null bit: This line can be wrapped. > string.h:247:33: note: nonnull attribute specified here > build_route_policies northd/northd.c:14771:32 > en_route_policies_run northd/en-northd.c:324:9 > engine_recompute lib/inc-proc-eng.c:448:33 > engine_compute lib/inc-proc-eng.c:491:17 > engine_run_node lib/inc-proc-eng.c:550:14 > engine_run ovn/lib/inc-proc-eng.c:579:9 > inc_proc_northd_run northd/inc-proc-northd.c:608:5 > main northd/ovn-northd.c:1134:36 > > Reported-at: https://issues.redhat.com/browse/FDP-2919 > Signed-off-by: Ales Musil <[email protected]> > --- > northd/northd.c | 5 +++++ > tests/ovn-northd.at | 28 ++++++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/northd/northd.c b/northd/northd.c > index c3c0780a3..2faf5d09a 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -14762,6 +14762,11 @@ build_route_policies(struct ovn_datapath *od, const > struct hmap *lr_ports, > for (size_t j = 0; j < n_nexthops; j++) { > char *nexthop = rule->n_nexthops > ? rule->nexthops[j] : rule->nexthop; > + > + if (!nexthop || !nexthop[0]) { I'm a little confused, how the nexthop can be NULL here? The default database value for a string is an empty string, which is not supposed to be NULL. And if n_nexthops is not zero, then we must have the rule->nexthops[j] non-NULL. What am I missing here? > + continue; > + } > + > struct ovn_port *out_port = NULL; > bool is_ipv4 = strchr(nexthop, '.') ? true : false; > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 5b1a8b6f8..79075465b 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -19030,3 +19030,31 @@ check ovn-nbctl --wait=sb sync > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([Router policies - crash]) This test name also doesn't provide any useful information about the test. > +ovn_start > + > +dnl Test with a simple reroute policy with an explicit output-port set. This comment should be converted into the test name, IMO. E.g.: AT_SETUP([Router policies - explicit output-port without nexthop]) > +check ovn-nbctl \ > + -- lr-add lr0 \ > + -- lrp-add lr0 lrp1 00:00:00:00:00:01 1.1.1.1/24 \ > + -- lrp-add lr0 lrp2 00:00:00:00:00:02 2.2.2.1/24 \ > + -- --output-port=lrp2 lr-policy-add lr0 100 "ip4.src == 42.42.42.42" \ > + reroute 3.3.3.3 > + > +dnl Clear the nexthops. This shouldn't crash and not produce any lflows. s/and not/or/ ? > +uuid=$(fetch_column nb:logical_router_policy _uuid priority=100) > +check ovn-nbctl clear logical_router_policy $uuid nexthops > +check ovn-nbctl --wait=sb sync > + > +ovn-sbctl dump-flows lr0 > lr0flows nit: Can be wrapped in AT_CHECK. > +AT_CAPTURE_FILE([lr0flows]) > + > +AT_CHECK([grep "lr_in_policy[[^_]]" lr0flows | ovn_strip_lflows | sort], > [0], [dnl > + table=??(lr_in_policy ), priority=0 , match=(1), > action=(reg8[[0..15]] = 0; next;) > +]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
