> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of David Harton
> Sent: Monday, August 7, 2017 6:39 PM
> To: tho...@monjalon.net
> Cc: dev@dpdk.org; David Harton <dhar...@cisco.com>
> 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 <dhar...@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 check
the return value. In theory the application could ignore the return value and
current behavior is maintained.
The validate-abi.sh script says "Compatible" with the following item flagged:
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
meaning.
Perhaps somebody with more ABI expertise than I would double check the return
value change?
Some smaller things inline below.
> 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 = &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 = 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.
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);
>
> /**
> * 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 like a
second opinion to Ack ABI.
-Harry
