From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 808947CD1 for ; Fri, 1 Sep 2017 10:01:35 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 0BA2C21C0A; Fri, 1 Sep 2017 04:01:35 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute1.internal (MEProxy); Fri, 01 Sep 2017 04:01:35 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=U3SaEqGW8KzqenQ dyHsf8WmsrZ2oEwDQlWMpaDIvqng=; b=leTm1VgCLzsBT7bg3TquB2Z8m/nNICL 4LDRFjrzHE+acjoUfE7g5RQBAC8IB8rBQCscszESTDEdcEAhZhe4C/2fhY6YIrWR M3+udxPAi67l5BTHQNhY3LfZNIkpoDf4EI3vzEitVbd8nAVxwLrYONQ5kEuRfkvN paOsCzDRhbMc= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s= fm1; bh=U3SaEqGW8KzqenQdyHsf8WmsrZ2oEwDQlWMpaDIvqng=; b=iRfxaNBn q0G6YhkfY0eGLriLkwgtaSYT9+dZYEoIgbwvA12bC7tNZdQq6To88uK3SxlwqD+7 TQ9KErxokptwDp8G8G9sSQ1I5tqpkh+JuX4l2iO7Grvj2BBwd7hlGasNC37fLXQy KBbTuKz/kAYcN9qF6HfJMaE0FPjEP6uQtM4LmcB9e24uxV5w0P1hhJNnHsojfKba PkBFr3FVacOs3dq0j6AtC/92HNk/PHBQ94EUNxSMGJVhPyUu/os5JKj0XBdMJq3W FuYYb6FxxaWdFLo+rrJO8ZMQHcHSFMyiVSJSfeB/7zPMNFmGgDSRdOhdKgY1brrZ 3Oi5rlfQNswZxg== X-ME-Sender: X-Sasl-enc: XqaG6J0lR9LM/P9CaCMhdNcWc/FNnnJSeOFkZSUBi3yV 1504252894 Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 93D777F44D; Fri, 1 Sep 2017 04:01:34 -0400 (EDT) From: Thomas Monjalon To: "David Harton (dharton)" Cc: dev@dpdk.org, ferruh.yigit@intel.com, ajit.khaparde@broadcom.com, johndale@cisco.com, konstantin.ananyev@intel.com, jingjing.wu@intel.com, beilei.xing@intel.com, jing.d.chen@intel.com, adrien.mazarguil@6wind.com, nelio.laranjeiro@6wind.com, alejandro.lucero@netronome.com, hemant.agrawal@nxp.com, rasesh.mody@cavium.com, harish.patil@cavium.com, skhare@vmware.com, yliu@fridaylinux.org, maxime.coquelin@redhat.com, allain.legacy@windriver.com Date: Fri, 01 Sep 2017 10:01:33 +0200 Message-ID: <3181043.jiBUBLgLyU@xps> In-Reply-To: References: <20170825133319.15207-1-dharton@cisco.com> <2775824.X3T22OWxbu@xps> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to return int X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Sep 2017 08:01:35 -0000 01/09/2017 02:40, David Harton (dharton): > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > --- a/doc/guides/rel_notes/release_17_11.rst > > > +++ b/doc/guides/rel_notes/release_17_11.rst > > > @@ -124,7 +124,7 @@ ABI Changes > > > Also, make sure to start the actual text at the margin. > > > ========================================================= > > > > > > - > > > +* Changed return type of ``vlan_offload_set_t`` from ``void`` to ``int``. > > > > It should be referenced as an API change (instead of ABI change). > > (and line spacing must be kept) > > Sure I'll move it to API. > > Line spacing? You mean 80 char width? I followed an example from a previous > release so I'm not sure what you mean. I mean you must take care of the number of blank lines to have before and after a title (2 blank lines before a title). > > > - (*dev->dev_ops->vlan_offload_set)(dev, mask); > > > + ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); > > > + if (ret) { > > > + /* hit an error restore original values */ > > > + dev->data->dev_conf.rxmode.hw_vlan_strip = org_strip; > > > + dev->data->dev_conf.rxmode.hw_vlan_filter = org_filter; > > > + dev->data->dev_conf.rxmode.hw_vlan_extend = org_extend; > > > + } > > > > Isn't it the responsibility of the PMD to restore values in case of error? > > I understand it is there to factorize error handling code, right? > > By setting the dev_conf.rxmode.hw_vlan_* fields this function is claiming > ownership of that data and therefore it should be the one to reset it. > > > Do we want to document this behaviour with the ops prototype? > > Why? Shouldn't any function that doesn't do what it was asked to do > leave the system in its original state and in fact if it doesn't that > is when it should be documented what the behavior is? Otherwise every > caller has to implement some cleanup code and would have to be given > information to know how to perform that cleanup as well which seems > more complicated. Sorry I overlooked the function. I thought these data were set by the PMD, that's why I was telling the cleanup should be done in the PMD. You can forget this comment :)