From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rcdn-iport-1.cisco.com (rcdn-iport-1.cisco.com [173.37.86.72]) by dpdk.org (Postfix) with ESMTP id 9728C2B88 for ; Tue, 8 Aug 2017 13:01:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=4310; q=dns/txt; s=iport; t=1502190097; x=1503399697; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=ALJ/qTdSOVH5GmgoP1+2Q3Y6r7mKMXZBRtA7iZlvt6s=; b=kBqx5MhXVeifiwKvD/o81kyc9A96BdHTdLhCE76/0E/1OlEC5W5Mas66 S1+8pHGMhzGNYHN/oph/2JVBkLEjgICnMJDf2eoZFucTDu71pJt/hKpCp eck7L+crXPbMBsxtmJ8A+QW2daMkC/osyWs01wwuoR/dIRNXnBVaeCrXN 4=; X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0DLAADpmIlZ/5JdJa1cGgEBAQECAQEBA?= =?us-ascii?q?QgBAQEBg1pkgRQHjgiQB4FukTuEWg6CBCSFIwKEaT8YAQIBAQEBAQEBayiFGAE?= =?us-ascii?q?BAQECAScTMQUJDAQCAQgRBAEBHwkHMhQJCAIEAQ0FCIofCK5COotgAQEBAQEBA?= =?us-ascii?q?QEBAQEBAQEBAQEBAQEBHYMoggKBTIN+gQyEXIYLBZd3iB8Ch1GMWpJWSJVCAR8?= =?us-ascii?q?4gQp3FYVdAxyBZ3YBh0QrgQWBDwEBAQ?= X-IronPort-AV: E=Sophos;i="5.41,343,1498521600"; d="scan'208";a="283634419" Received: from rcdn-core-10.cisco.com ([173.37.93.146]) by rcdn-iport-1.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Aug 2017 11:01:36 +0000 Received: from XCH-ALN-019.cisco.com (xch-aln-019.cisco.com [173.36.7.29]) by rcdn-core-10.cisco.com (8.14.5/8.14.5) with ESMTP id v78B1aA6015186 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Tue, 8 Aug 2017 11:01:36 GMT Received: from xch-rcd-016.cisco.com (173.37.102.26) by XCH-ALN-019.cisco.com (173.36.7.29) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Tue, 8 Aug 2017 06:01:35 -0500 Received: from xch-rcd-016.cisco.com ([173.37.102.26]) by XCH-RCD-016.cisco.com ([173.37.102.26]) with mapi id 15.00.1210.000; Tue, 8 Aug 2017 06:01:35 -0500 From: "David Harton (dharton)" To: "Van Haaren, Harry" , "thomas@monjalon.net" CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v3] ethdev: add return code to rte_eth_stats_reset() Thread-Index: AQHTECVTGg3vxy+lJkGmcWxsk0Se3qJ6Rr/g Date: Tue, 8 Aug 2017 11:01:35 +0000 Message-ID: <2db3319155e641ea99eb7756e4ef812f@XCH-RCD-016.cisco.com> References: <20170807173914.36750-1-dharton@cisco.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.82.247.104] 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 11:01:38 -0000 > -----Original Message----- > From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com] > Sent: Tuesday, August 08, 2017 5:03 AM > To: David Harton (dharton) ; thomas@monjalon.net > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v3] ethdev: add return code to > rte_eth_stats_reset() >=20 > > 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_reset() > > > > Some devices do not support reset of eth stats. An application may > > need to know not to clear shadow stats if the device cannot. > > > > rte_eth_stats_reset is updated to provide a return code to share > > whether the device supports reset or not. > > > > Signed-off-by: David Harton > > --- >=20 > Hi, >=20 > As far as I know changing the return type (void to int) of a function doe= s > *not* break ABI, but does "break" API as the application code should now > check the return value. In theory the application could ignore the return > value and current behavior is maintained. >=20 > The validate-abi.sh script says "Compatible" with the following item > flagged: >=20 > Problems with Symbols > High 0 > Medium 0 > Low 1 >=20 > 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 > meaning. >=20 > Perhaps somebody with more ABI expertise than I would double check the > return value change? >=20 >=20 > Some smaller things inline below. >=20 > > v3: > > * overcame noob errors and figured out patch challenges > > * this release should finally be clean :) > > > > v2: > > * fixed soft tab issue inserted while moving changes > > > > lib/librte_ether/rte_ethdev.c | 8 +++++--- > > lib/librte_ether/rte_ethdev.h | 4 +++- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > 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; > > } > > > > -void > > +int > > rte_eth_stats_reset(uint8_t port_id) > > { > > struct rte_eth_dev *dev; > > > > - 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]; > > > > - 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; > > } > > > > 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. >=20 > We should document all return values: >=20 > @retval 0 Success > @retval -EINVAL Invalid port_id provided @retval -ENOTSUP Stats reset > functionality not supported by PMD Sure. I was following the convention of function above it rte_eth_stats_get() but I agree better to advertise externally. I'll also modify the port number check errval to -ENODEV. >=20 > The API change itself should probably be added to release notes, as > applications may wish to be aware this function has changed. Sounds good. >=20 > > */ > > -void rte_eth_stats_reset(uint8_t port_id); > > +int rte_eth_stats_reset(uint8_t port_id); > > > > /** > > * Retrieve names of extended statistics of an Ethernet device. > > -- > > 2.10.3.dirty >=20 >=20 > I'm happy to Ack the code/release-notes with above suggestions, but I'd > like a second opinion to Ack ABI. Thanks for the review, Dave >=20 > -Harry