On Wed, Dec 13, 2023 at 7:09 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 11/28/23 03:36, num...@ovn.org wrote:
>
> [...]
>
> > +
> > +/* lflow ref mgr */
> > +struct lflow_ref {
> > + char *res_name;
> > +
> > + /* head of the list 'struct lflow_ref_node'. */
> > + struct ovs_list lflows_ref_list;
> > +
> > + /* hmapx_node is 'struct lflow *'. This is used to ensure
> > + * that there are no duplicates in 'lflow_ref_list' above. */
> > + struct hmapx lflows;
> > +};
> > +
> > +struct lflow_ref_node {
> > + struct ovs_list ref_list_node;
> > +
> > + struct ovn_lflow *lflow;
> > + size_t dp_index;
> > +};
> > +
> > +struct lflow_ref *
> > +lflow_ref_alloc(const char *res_name)
> > +{
>
> This does more than just allocating the lflow_ref. For consistency this
> should be called lflow_ref_create(..).
Ack. I'll handle it in v4.
>
> > + struct lflow_ref *lflow_ref = xzalloc(sizeof *lflow_ref);
> > + lflow_ref->res_name = xstrdup(res_name);
> > + ovs_list_init(&lflow_ref->lflows_ref_list);
> > + hmapx_init(&lflow_ref->lflows);
> > + return lflow_ref;
> > +}
> > +
> > +void
> > +lflow_ref_destroy(struct lflow_ref *lflow_ref)
> > +{
> > + free(lflow_ref->res_name);
> > +
> > + struct lflow_ref_node *l;
> > + LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow_ref->lflows_ref_list) {
> > + ovs_list_remove(&l->ref_list_node);
> > + free(l);
> > + }
> > +
> > + hmapx_destroy(&lflow_ref->lflows);
> > + free(lflow_ref);
> > +}
> > +
> > +void
> > +lflow_ref_reset(struct lflow_ref *lflow_ref)
>
> IMO lflow_ref_clear() might be a better name.
>
> > +{
> > + struct lflow_ref_node *l;
> > + LIST_FOR_EACH_SAFE (l, ref_list_node, &lflow_ref->lflows_ref_list) {
> > + ovs_list_remove(&l->ref_list_node);
> > + free(l);
> > + }
> > +
> > + hmapx_clear(&lflow_ref->lflows);
> > +}
> > +
> > +void
> > +lflow_ref_clear_lflows(struct lflow_ref *lflow_ref)
>
> This doesn't clear, it just "unlinks" (clears all set datapath indices
> in the dp bitmap) flows. Should we remove the static
> unlink_lflows_from_datapath() function and inline it here and then call
> this lflow_ref_unlink_lflows()?
Ack. I'll handle it in v4.
>
> > +{
> > + unlink_lflows_from_datapath(lflow_ref);
> > +}
> > +
> > +void
> > +lflow_ref_clear_and_sync_lflows(struct lflow_ref *lflow_ref,
>
> I think a more accurate and concise name is lflow_ref_resync_flows().
Ack. I'll handle it in v4.
>
> > + struct lflow_table *lflow_table,
> > + struct ovsdb_idl_txn *ovnsb_txn,
> > + const struct ovn_datapaths *ls_datapaths,
> > + const struct ovn_datapaths *lr_datapaths,
> > + bool ovn_internal_version_changed,
> > + const struct sbrec_logical_flow_table *sbflow_table,
> > + const struct sbrec_logical_dp_group_table *dpgrp_table)
> > +{
> > + unlink_lflows_from_datapath(lflow_ref);
>
> This could be lflow_ref_unlink_lflows() instead.
Ack. I'll handle it in v4.
>
> > + lflow_ref_sync_lflows_to_sb__(lflow_ref, lflow_table, ovnsb_txn,
> > + ls_datapaths, lr_datapaths,
> > + ovn_internal_version_changed,
> > sbflow_table,
> > + dpgrp_table);
> > +}
> > +
> > +void
> > +lflow_ref_sync_lflows_to_sb(struct lflow_ref *lflow_ref,
>
> IMO it's enough to call this lflow_ref_sync_lflows(). It's implied that
> we're syncing to the SB.
Ack. I'll handle it in v4.
>
> > + struct lflow_table *lflow_table,
> > + struct ovsdb_idl_txn *ovnsb_txn,
> > + const struct ovn_datapaths *ls_datapaths,
> > + const struct ovn_datapaths *lr_datapaths,
> > + bool ovn_internal_version_changed,
> > + const struct sbrec_logical_flow_table *sbflow_table,
> > + const struct sbrec_logical_dp_group_table
> > *dpgrp_table)
> > +{
> > + lflow_ref_sync_lflows_to_sb__(lflow_ref, lflow_table, ovnsb_txn,
> > + ls_datapaths, lr_datapaths,
> > + ovn_internal_version_changed,
> > sbflow_table,
> > + dpgrp_table);
> > +}
> > +
>
> [...]
>
> > +
> > +static void
> > +lflow_ref_sync_lflows_to_sb__(struct lflow_ref *lflow_ref,
> > + struct lflow_table *lflow_table,
> > + struct ovsdb_idl_txn *ovnsb_txn,
> > + const struct ovn_datapaths *ls_datapaths,
> > + const struct ovn_datapaths *lr_datapaths,
> > + bool ovn_internal_version_changed,
> > + const struct sbrec_logical_flow_table
> > *sbflow_table,
> > + const struct sbrec_logical_dp_group_table
> > *dpgrp_table)
>
> IMO lflow_ref_sync_lflows__() is explicit enough as a name.
Ack. I'll handle it in v4.
Thanks
Numan
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
