On 1/5/26 3:13 PM, Ales Musil wrote: > > > On Mon, Jan 5, 2026 at 12:53 PM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: > > 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. > > > Hi Ilya, > thank you for the review. > > > > 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. > > > That's fair, updated. > > > > > > > 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 > <https://issues.redhat.com/browse/FDP-2919> > > Signed-off-by: Ales Musil <[email protected] <mailto:[email protected]>> > > --- > > northd/northd.c | 5 +++++ > > tests/ovn-northd.at <http://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? > > > Both nexthop and nexthops columns are defined as "min": 0, > so please correct if I'm wrong, but that means it can also be NULL, > right?
Ah, yeah, you're right. I looked at the static route table instead, where we don't have a min value. > > > > > + continue; > > + } > > + > > struct ovn_port *out_port = NULL; > > bool is_ipv4 = strchr(nexthop, '.') ? true : false; > > > > diff --git a/tests/ovn-northd.at <http://ovn-northd.at> > b/tests/ovn-northd.at <http://ovn-northd.at> > > index 5b1a8b6f8..79075465b 100644 > > --- a/tests/ovn-northd.at <http://ovn-northd.at> > > +++ b/tests/ovn-northd.at <http://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 > <http://1.1.1.1/24> \ > > + -- lrp-add lr0 lrp2 00:00:00:00:00:02 2.2.2.1/24 > <http://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 > > +]) > > > All nits will be addressed in v2. > > Regards, > Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
