[dpdk-dev] [PATCH v2 06/25] app/testpmd: implement basic support for rte_flow

[Xing, Beilei]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Tuesday, December 20, 2016 5:38 PM
> To: Xing, Beilei <beilei.xing at intel.com>
> Cc: dev at dpdk.org; Pei, Yulong <yulong.pei at intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 06/25] app/testpmd: implement basic
> support for rte_flow
> 
> On Tue, Dec 20, 2016 at 01:57:46AM +0000, Xing, Beilei wrote:
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> > > Sent: Monday, December 19, 2016 6:20 PM
> > > To: Xing, Beilei <beilei.xing at intel.com>
> > > Cc: dev at dpdk.org; Pei, Yulong <yulong.pei at intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2 06/25] app/testpmd: implement
> > > basic support for rte_flow
> > >
> > > Hi Beilei,
> > >
> > > On Mon, Dec 19, 2016 at 08:37:20AM +0000, Xing, Beilei wrote:
> > > > Hi Adrien,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Adrien
> > > > > Mazarguil
> > > > > Sent: Saturday, December 17, 2016 12:25 AM
> > > > > To: dev at dpdk.org
> > > > > Subject: [dpdk-dev] [PATCH v2 06/25] app/testpmd: implement
> > > > > basic support for rte_flow
> > > > >
> > > > > Add basic management functions for the generic flow API
> > > > > (validate, create, destroy, flush, query and list). Flow rule
> > > > > objects and properties are arranged in lists associated with each port.
> > > > >
> > > > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil at 6wind.com>
> > > > > +/** Create flow rule. */
> > > > > +int
> > > > > +port_flow_create(portid_t port_id,
> > > > > +		 const struct rte_flow_attr *attr,
> > > > > +		 const struct rte_flow_item *pattern,
> > > > > +		 const struct rte_flow_action *actions) {
> > > > > +	struct rte_flow *flow;
> > > > > +	struct rte_port *port;
> > > > > +	struct port_flow *pf;
> > > > > +	uint32_t id;
> > > > > +	struct rte_flow_error error;
> > > > > +
> > > >
> > > > I think there should be memset for error here, e.g. memset(&error,
> > > > 0, sizeof(struct rte_flow_error)); Since both cause and message
> > > > may be NULL
> > > regardless of the error type, if there's no error.cause and
> > > error.message returned from PMD, Segmentation fault will happen in
> port_flow_complain.
> > > > PS: This issue doesn't happen if add "export EXTRA_CFLAGS=' -g
> > > > O0'" when
> > > compiling.
> > >
> > > Actually, PMDs must fill the error structure only in case of error
> > > if the application provides one, it's not optional. I didn't
> > > initialize this structure for this reason.
> > >
> > > I suggest we initialize it with a known poisoning value for
> > > debugging purposes though, to make it fail every time. Does it sound
> reasonable?
> 
> Done for v3 by the way.
> 
> > OK, I see. Do you want PMD to allocate the memory for cause and message
> of error, and must fill the cause and message if error exists, right?
> > So is it possible to set NULL for pointers of cause and message in application?
> then PMD can judge if it's need to allocate or overlap memory.
> 
> PMDs never allocate this structure, applications do and initialize it however
> they want. They only provide a non-NULL pointer if they want additional
> details in case of error.
> 
> It will likely be allocated on the stack in most cases (as in testpmd).
> 
> From a PMD standpoint, the contents of this structure must be updated in
> case of non-NULL pointer and error state.
> 
> Now the message pointer can be allocated dynamically but it's not
> recommended, it's far easier to make it point to some constant string.
> Applications won't free it anyway, so PMDs would have to do it during
> dev_close(). Please see "Verbose error reporting" documentation section.

Got it, thanks. Seems the rte_flow_error_set function can be used for PMD.

Regards,
Beilei