From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 56C4F2986 for ; Tue, 8 Aug 2017 11:04:16 +0200 (CEST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Aug 2017 02:04:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,342,1498546800"; d="scan'208";a="136988135" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by fmsmga005.fm.intel.com with ESMTP; 08 Aug 2017 02:04:14 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.211]) by IRSMSX107.ger.corp.intel.com ([169.254.10.129]) with mapi id 14.03.0319.002; Tue, 8 Aug 2017 10:02:49 +0100 From: "Van Haaren, Harry" To: David Harton , "thomas@monjalon.net" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v3] ethdev: add return code to rte_eth_stats_reset() Thread-Index: AQHTD6QsmoEusHnlk020e9LVvIkYUaJ6JUGw Date: Tue, 8 Aug 2017 09:02:49 +0000 Message-ID: References: <20170807173914.36750-1-dharton@cisco.com> In-Reply-To: <20170807173914.36750-1-dharton@cisco.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiN2NmZDJjMTQtNGY0Yi00YWU3LTg5NjMtOWZlZjA3MTFmNTY2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjRzbTlDYUJ4b1pKWXZQWmp3Y3ZnWnJrVHRhWVwvcmQ3UHFYTGZNM1ZJYTFVPSJ9 x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3] ethdev: add return code to rte_eth_stats_reset() 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: Tue, 08 Aug 2017 09:04:17 -0000 > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Harton > Sent: Monday, August 7, 2017 6:39 PM > To: thomas@monjalon.net > Cc: dev@dpdk.org; David Harton > Subject: [dpdk-dev] [PATCH v3] ethdev: add return code to rte_eth_stats_r= eset() >=20 > Some devices do not support reset of eth stats. An application may > need to know not to clear shadow stats if the device cannot. >=20 > rte_eth_stats_reset is updated to provide a return code to share > whether the device supports reset or not. >=20 > Signed-off-by: David Harton > --- Hi, As far as I know changing the return type (void to int) of a function does = *not* break ABI, but does "break" API as the application code should now ch= eck the return value. In theory the application could ignore the return val= ue and current behavior is maintained. The validate-abi.sh script says "Compatible" with the following item flagge= d: Problems with Symbols High 0 Medium 0 Low 1 Change>> Type of return value has been changed from void to int (4 bytes). Effect>> Replacement of return type may indicate a change in its semantic m= eaning. Perhaps somebody with more ABI expertise than I would double check the retu= rn value change? Some smaller things inline below. > v3: > * overcame noob errors and figured out patch challenges > * this release should finally be clean :) >=20 > v2: > * fixed soft tab issue inserted while moving changes >=20 > lib/librte_ether/rte_ethdev.c | 8 +++++--- > lib/librte_ether/rte_ethdev.h | 4 +++- > 2 files changed, 8 insertions(+), 4 deletions(-) >=20 > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.= c > index 0597641..f72cc5a 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -1341,17 +1341,19 @@ rte_eth_stats_get(uint8_t port_id, struct rte_eth= _stats *stats) > return 0; > } >=20 > -void > +int > rte_eth_stats_reset(uint8_t port_id) > { > struct rte_eth_dev *dev; >=20 > - RTE_ETH_VALID_PORTID_OR_RET(port_id); > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > dev =3D &rte_eth_devices[port_id]; >=20 > - RTE_FUNC_PTR_OR_RET(*dev->dev_ops->stats_reset); > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_reset, -ENOTSUP); > (*dev->dev_ops->stats_reset)(dev); > dev->data->rx_mbuf_alloc_failed =3D 0; > + > + return 0; > } >=20 > static int > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.= h > index 0adf327..d8ccd60 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -2246,8 +2246,10 @@ int rte_eth_stats_get(uint8_t port_id, struct rte_= eth_stats > *stats); > * > * @param port_id > * The port identifier of the Ethernet device. > + * @return > + * Zero if successful. Non-zero otherwise. We should document all return values: @retval 0 Success @retval -EINVAL Invalid port_id provided @retval -ENOTSUP Stats reset functionality not supported by PMD The API change itself should probably be added to release notes, as applications may wish to be aware this function has changed. > */ > -void rte_eth_stats_reset(uint8_t port_id); > +int rte_eth_stats_reset(uint8_t port_id); >=20 > /** > * Retrieve names of extended statistics of an Ethernet device. > -- > 2.10.3.dirty I'm happy to Ack the code/release-notes with above suggestions, but I'd lik= e a second opinion to Ack ABI. -Harry