> -----Original Message----- > From: Van Haaren, Harry [mailto:harry.van.haa...@intel.com] > Sent: Tuesday, August 08, 2017 5:03 AM > To: David Harton (dharton) <dhar...@cisco.com>; tho...@monjalon.net > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v3] ethdev: add return code to > rte_eth_stats_reset() > > > 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
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. > > The API change itself should probably be added to release notes, as > applications may wish to be aware this function has changed. Sounds good. > > > */ > > -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. Thanks for the review, Dave > > -Harry
