Thanks Xavier and Dumitru. I addressed the nits and pushed the change to main and branch-25.09.
On Thu, Dec 4, 2025 at 5:56 AM Dumitru Ceara via dev <[email protected]> wrote: > > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
