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(
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
