Re: [PATCH v2 06/25] app/testpmd: implement basic support for rte_flow

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Tue, Dec 20, 2016 at 01:57:46AM +0000, Xing, Beilei wrote:
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Monday, December 19, 2016 6:20 PM
> > To: Xing, Beilei <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; Pei, Yulong <yulong.pei@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@dpdk.org] On Behalf Of Adrien
> > > > Mazarguil
> > > > Sent: Saturday, December 17, 2016 12:25 AM
> > > > To: dev@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@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.

-- 
Adrien Mazarguil
6WIND