Hi Xavier, Thanks for v2!
On 12/3/25 3:54 PM, Xavier Simonart wrote: > sb_port_group_lookup_by_name() can return NULL if the (stale) record > it's searching for has been deleted in transactions that are processed > in the current iteration. Nit (but this can be addressed by whoever merges the patch): I'd add an empty line here. > This caused segfault such as: > ovsdb_idl_row_is_synthetic (row=0x0) at lib/ovsdb-idl.c:2724 > ovsdb_idl_txn_delete (row_=0x0) at lib/ovsdb-idl.c:3834 > port_group_nb_port_group_handler (node=<optimized out>, data_=<optimized > out>) at northd/en-port-group.c:616 > ... > > Signed-off-by: Xavier Simonart <[email protected]> > Fixes: a1d82e9fa11e ("northd: Process port_group changes incrementally.") Nit (but this can be addressed by whoever merges the patch): the Signed-off-by should be the last line in the commit log. > > --- > -v2: based on Dumitru's feedback: > - Updated commit message. > - Update test case. > --- > northd/en-port-group.c | 2 ++ > tests/ovn-northd.at | 27 +++++++++++++++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/northd/en-port-group.c b/northd/en-port-group.c > index 476c0a18d..d0b7961fb 100644 > --- a/northd/en-port-group.c > +++ b/northd/en-port-group.c > @@ -613,7 +613,9 @@ port_group_nb_port_group_handler(struct engine_node > *node, void *data_) > const struct sbrec_port_group *sb_pg = > sb_port_group_lookup_by_name(sbrec_port_group_by_name, > stale_sb_port_group_name); > + if (sb_pg) { > sbrec_port_group_delete(sb_pg); > + } > } > > sset_destroy(&stale_sb_port_groups); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 931064fe6..168e34f7e 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -18914,3 +18914,30 @@ wait_row_count Igmp_Group 0 address=mrouters > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([port group segfault]) > + > +ovn_start > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lsp-add sw0 sw0-port1 \ > + -- lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2" > + > +check ovn-nbctl pg-add pg1 > +check ovn-nbctl --wait=sb pg-set-ports pg1 sw0-port1 > + > +sleep_sb > +sleep_northd > +check ovn-nbctl pg-del pg1 > +check ovn-nbctl pg-add pg1 > +check ovn-nbctl pg-set-ports pg1 sw0-port1 > + > +wake_up_sb > +wake_up_northd > +check ovn-nbctl pg-del pg1 > +check ovn-nbctl --wait=sb sync > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) The change looks good to me, thanks! Acked-by: Dumitru Ceara <[email protected]> Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
