Re: [dpdk-dev] [PATCH v3 1/8] app/testpmd: store VXLAN/NVGRE encap data globally

["Jiawei(Jonny) Wang" <jiaweiw@nvidia.com>]

Hello Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, March 31, 2021 8:08 PM
> To: Salem Sol <salems@nvidia.com>; dev@dpdk.org
> Cc: Jiawei(Jonny) Wang <jiaweiw@nvidia.com>; Ori Kam <orika@nvidia.com>;
> Xiaoyun Li <xiaoyun.li@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/8] app/testpmd: store VXLAN/NVGRE
> encap data globally
> 
> On 3/17/2021 9:26 AM, Salem Sol wrote:
> > From: Jiawei Wang <jiaweiw@nvidia.com>
> >
> > With the current code the VXLAN/NVGRE parsing routine stored the
> > configuration of the header on stack, this might lead to overwriting
> > the data on the stack.
> >
> > This patch stores the external data of vxlan and nvgre encap into
> > global data as a pre-step to supporting vxlan and nvgre encap as a
> > sample actions.
> >
> 
> I didn't get what was on stack and moved in to the global data, can you
> please elaborate.
> 

This patch split the function and saved input data into input parameter:
So it mentioned here "pre-step" for next store the data of vxlan/nvgre into global.

The global var for sample action is defined in: 
(https://patches.dpdk.org/project/dpdk/patch/20210317092610.71000-5-salems@nvidia.com/)
struct action_vxlan_encap_data sample_vxlan_encap[RAW_SAMPLE_CONFS_MAX_NUM];

> For example for nvgre, 'action_nvgre_encap_data' is pointer in stack but the
> actual object is 'ctx->object', so it is not really in the stack.
> 

After call 'set sample 0 nvgre .. ', the data be stored into 'ctx->object', the 'ctx->object' will be reused
for the following CLI command, After that, while we try to use previous 'sample action' to fetch nvgre data,
these data may be lost.

For sample action, use global data can save the previous nvgre configurations data.

> Tough, OK to refactor and split the function as preparation to support the
> sample action.
> 
> > Signed-off-by: Jiawei Wang <jiaweiw@nvidia.com>
> 
> <...>
> 
> > -/** Parse VXLAN encap action. */
> > +/** Setup VXLAN encap configuration. */
> >   static int
> > -parse_vc_action_vxlan_encap(struct context *ctx, const struct token
> *token,
> > -			    const char *str, unsigned int len,
> > -			    void *buf, unsigned int size)
> > +parse_setup_vxlan_encap_data
> > +		(struct action_vxlan_encap_data *action_vxlan_encap_data)
> 
> Can you please fix the syntax, I guess this is done to keep within in 80 column
> limit, but from readability perspective I think better to go over the 80 column
> a little instead of breaking the line like this.
> 

Ok, will change into one line.

> <...>
> 
> > +/** Setup NVGRE encap configuration. */ static int
> > +parse_setup_nvgre_encap_data
> > +		(struct action_nvgre_encap_data *action_nvgre_encap_data)
> {
> > +	if (!action_nvgre_encap_data)
> > +		return -1;
> 
> This is a static function, and the input of it is in our control, so not sure if we
> should verify the input here.
> But if we need to, can you please check the return value of this function
> where it is called.

I agree with you that can remove this checking inside, since we make sure it's valid before call.

Thanks.