On 03.10.2025 15:26, Dumitru Ceara wrote: On 9/30/25 4:06 PM, Alexandra Rukomoinikova wrote:
Change svc_monitors_map_data_init() to take an output parameter instead of returning a struct by value. Moved functions to more appropriate locations Signed-off-by: Alexandra Rukomoinikova <[email protected]><mailto:[email protected]> --- Hi Alexandra, Thanks for the patch. northd/en-lflow.c | 5 +++-- northd/northd.c | 47 +++++++++++++++++++++++++---------------------- northd/northd.h | 10 +++++----- 3 files changed, 33 insertions(+), 29 deletions(-) diff --git a/northd/en-lflow.c b/northd/en-lflow.c index e80fd9f9c..48076c8e0 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -344,8 +344,9 @@ lflow_ic_learned_svc_mons_handler(struct engine_node *node, struct lflow_input lflow_input; lflow_get_input_data(node, &lflow_input); - struct svc_monitors_map_data svc_mons_data = - svc_monitors_map_data_init( + struct svc_monitors_map_data svc_mons_data; + svc_monitors_map_data_init( + &svc_mons_data, NULL, &ic_learned_svc_monitors_data->ic_learned_svc_monitors_map, ic_learned_svc_monitors_data->lflow_ref); diff --git a/northd/northd.c b/northd/northd.c index 0c3384fe9..1f1b5aa83 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -713,18 +713,6 @@ init_mcast_info_for_datapath(struct ovn_datapath *od) } } -struct svc_monitors_map_data -svc_monitors_map_data_init(const struct hmap *local_svc_monitors_map, - const struct hmap *ic_learned_svc_monitors_map, - struct lflow_ref *ic_learned_svc_monitors_lflow_ref) -{ - return (struct svc_monitors_map_data) { - .local_svc_monitors_map = local_svc_monitors_map, - .ic_learned_svc_monitors_map = ic_learned_svc_monitors_map, - .lflow_ref = ic_learned_svc_monitors_lflow_ref, - }; -} I'm not sure I understand the problem your patch is trying to fix. We are indeed returning the resulting struct by value (copying its fields values). But that's OK, they're all pointers. - static void destroy_mcast_info_for_switch_datapath(struct ovn_datapath *od) { @@ -18065,7 +18053,8 @@ build_lflows_thread(void *arg) return NULL; } struct svc_monitors_map_data svc_mons_data; - svc_mons_data = svc_monitors_map_data_init( + svc_monitors_map_data_init( + &svc_mons_data, lsi->local_svc_monitor_map, lsi->ic_learned_svc_monitor_map, NULL); @@ -18425,11 +18414,12 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, struct lflow_input *input_data, struct lflow_table *lflows) { - struct svc_monitors_map_data svc_mons_data = - svc_monitors_map_data_init( - input_data->local_svc_monitors_map, - input_data->ic_learned_svc_monitors_map, - input_data->ic_learned_svc_monitors_lflow_ref); + struct svc_monitors_map_data svc_mons_data; + svc_monitors_map_data_init( + &svc_mons_data, + input_data->local_svc_monitors_map, + input_data->ic_learned_svc_monitors_map, + input_data->ic_learned_svc_monitors_lflow_ref); In my opinion the old version was more readable because it explicitly represents the "assignment" semantics. Unless there's an actual functionality bug that we need to fix I'd keep the old version. Regards, Dumitru build_lswitch_and_lrouter_flows(input_data->ls_datapaths, input_data->lr_datapaths, @@ -18722,10 +18712,12 @@ lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, struct ovn_lb_datapaths *lb_dps; struct hmapx_node *hmapx_node; - struct svc_monitors_map_data svc_mons_data = - svc_monitors_map_data_init(lflow_input->local_svc_monitors_map, - lflow_input->ic_learned_svc_monitors_map, - NULL); + struct svc_monitors_map_data svc_mons_data; + svc_monitors_map_data_init( + &svc_mons_data, + lflow_input->local_svc_monitors_map, + lflow_input->ic_learned_svc_monitors_map, + NULL); HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) { lb_dps = hmapx_node->data; @@ -19361,6 +19353,17 @@ bfd_sync_destroy(struct bfd_sync_data *data) sset_destroy(&data->bfd_ports); } +void +svc_monitors_map_data_init(struct svc_monitors_map_data *svc_mons_data, + const struct hmap *local_svc_monitors_map, + const struct hmap *ic_learned_svc_monitors_map, + struct lflow_ref *ic_learned_svc_monitors_lflow_ref) +{ + svc_mons_data->local_svc_monitors_map = local_svc_monitors_map; + svc_mons_data->ic_learned_svc_monitors_map = ic_learned_svc_monitors_map; + svc_mons_data->lflow_ref = ic_learned_svc_monitors_lflow_ref; +} + void ic_learned_svc_monitors_init(struct ic_learned_svc_monitors_data *data) { diff --git a/northd/northd.h b/northd/northd.h index b095838c3..8b6ba923f 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -242,11 +242,6 @@ struct ic_learned_svc_monitors_data { struct lflow_ref *lflow_ref; }; -struct svc_monitors_map_data -svc_monitors_map_data_init(const struct hmap *local_svc_monitors_map, - const struct hmap *ic_learned_svc_monitors_map, - struct lflow_ref *ic_learned_svc_monitors_lflow_ref); - struct lflow_ref; struct lr_nat_table; @@ -903,6 +898,11 @@ void bfd_sync_init(struct bfd_sync_data *); void bfd_sync_swap(struct bfd_sync_data *, struct sset *bfd_ports); void bfd_sync_destroy(struct bfd_sync_data *); +void +svc_monitors_map_data_init(struct svc_monitors_map_data *svc_mons_data, + const struct hmap *local_svc_monitors_map, + const struct hmap *ic_learned_svc_monitors_map, + struct lflow_ref *ic_learned_svc_monitors_lflow_ref); void ic_learned_svc_monitors_init( struct ic_learned_svc_monitors_data *data); void ic_learned_svc_monitors_cleanup( Hi! Thanks for the review! I wrote this old code that I wanted to fix in this patch. I understand there's no problem—they're all pointers. Just looking at this old code now, I thought I'd fix it, because according to generally accepted standards, in northd code, it literally calculates the location where it's done (returned by value). Overall, I have no problem with this patch not being accepted. =) -- regards, Alexandra. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
