Hi Beilei,
On Mon, Dec 19, 2016 at 08:37:20AM +0000, Xing, Beilei wrote:
> Hi Adrien,
>
> > -----Original Message-----
> > From: dev [mailto:dev-boun...@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.mazarg...@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
