From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <harry.van.haaren@intel.com>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
 by dpdk.org (Postfix) with ESMTP id 56C4F2986
 for <dev@dpdk.org>; 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" <harry.van.haaren@intel.com>
To: David Harton <dharton@cisco.com>, "thomas@monjalon.net"
 <thomas@monjalon.net>
CC: "dev@dpdk.org" <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: <E923DB57A917B54B9182A2E928D00FA640C45B4F@IRSMSX102.ger.corp.intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=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 <dharton@cisco.com>
> 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 <dharton@cisco.com>
> ---

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