From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v2 06/25] app/testpmd: implement basic support for rte_flow Date: Tue, 20 Dec 2016 10:38:15 +0100 Message-ID: <20161220093815.GO10340@6wind.com> References: <3318a43c9e105caaf4d5b13d6e4fcce774fb522f.1481903839.git.adrien.mazarguil@6wind.com> <94479800C636CB44BD422CB454846E013157433C@SHSMSX101.ccr.corp.intel.com> <20161219101943.GJ10340@6wind.com> <94479800C636CB44BD422CB454846E0131574785@SHSMSX101.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "dev@dpdk.org" , "Pei, Yulong" To: "Xing, Beilei" Return-path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 0E053FAE8 for ; Tue, 20 Dec 2016 10:38:24 +0100 (CET) Received: by mail-wm0-f46.google.com with SMTP id f82so124962131wmf.1 for ; Tue, 20 Dec 2016 01:38:24 -0800 (PST) Content-Disposition: inline In-Reply-To: <94479800C636CB44BD422CB454846E0131574785@SHSMSX101.ccr.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > Cc: dev@dpdk.org; Pei, Yulong > > 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 > > > > +/** 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