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

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

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?

> > +	flow = rte_flow_create(port_id, attr, pattern, actions, &error);
> > +	if (!flow)
> > +		return port_flow_complain(&error);
> > +	port = &ports[port_id];
> > +	if (port->flow_list) {
> > +		if (port->flow_list->id == UINT32_MAX) {
> > +			printf("Highest rule ID is already assigned, delete"
> > +			       " it first");
> > +			rte_flow_destroy(port_id, flow, NULL);
> > +			return -ENOMEM;
> > +		}
> > +		id = port->flow_list->id + 1;
> > +	} else
> > +		id = 0;
> > +	pf = port_flow_new(attr, pattern, actions);
> > +	if (!pf) {
> > +		int err = rte_errno;
> > +
> > +		printf("Cannot allocate flow: %s\n", rte_strerror(err));
> > +		rte_flow_destroy(port_id, flow, NULL);
> > +		return -err;
> > +	}
> > +	pf->next = port->flow_list;
> > +	pf->id = id;
> > +	pf->flow = flow;
> > +	port->flow_list = pf;
> > +	printf("Flow rule #%u created\n", pf->id);
> > +	return 0;
> > +}
> > +

-- 
Adrien Mazarguil
6WIND