Re: [dpdk-dev] [RFC] add flow action context API

[Thomas Monjalon <thomas@monjalon.net>]

04/06/2020 13:12, Andrey Vesnovaty:
> Thomas Monjalon <thomas@monjalon.net> wrote:
> > 20/05/2020 11:18, Andrey Vesnovaty:
> > We had "create", "destroy", "query", but no "modify" capability.
> > The new API is adding 2 things in my opinion:
> >         - shared action object
> >         - "modify" capability (is "update" a better wording?)
> 
> Naming is one of the most challenging parts of this RFC.
> Some similarity I have found in existing code is
> rte_mtr_policer_actions_update()
> Is there any existing code having update/modify semantics?

Except one callback in librte_fib, no DPDK API has "modify" in its name.
You can find the word "update" in the API of multiple DPDK libs.
I would like having the opinion of a native english speaker here.


> > About the wording, do we need "ctx"?
> > I feel rte_flow_action is a good enough prefix for this API,
> > and should be documented as a shared action object.
> > I think the word "object" is more meaningful than "context".
> > Am I missing something?
> 
> CTX comes for the fact that each flow_rule doesn't have an ownership for
> the given action but operates inside some context (shared action context
> actually).
> As mentioned above, naming is one of the most challenging parts of this
> RFC.

I think we can replace "context" with "object" in the explanations.
The functions could be named without "ctx":
	- rte_flow_action_create
	- rte_flow_action_destroy
	- rte_flow_action_update
	- rte_flow_action_query


> > > +     /**
> > > +      * Describes action context.
> > > +      *
> > > +      * Enables multiple rules reference the same action by id/ctx.
> > > +      *
> > > +      * No action specific struct here (void*) since it can be any
> > > +      * action type.
> > > +      */
> > > +     RTE_FLOW_ACTION_TYPE_CTX,
> >
> > Why do we need a new action type?
> >
> Because it's not an action itself but a reference/handle to it.

Sorry I don't understand when RTE_FLOW_ACTION_TYPE_CTX has to be used.
There is no mention of RTE_FLOW_ACTION_TYPE_CTX in the explanations.