On 10/3/25 2:34 PM, Rukomoinikova Aleksandra wrote: > 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]> >>> --- >> 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. =) >
In this case I'd rather keep it as you originally wrote it. It seems better than the alternative. Thank you! Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
