Hi Olivier, I apologize for so long delay with reply. I simply lost it from my view.
Many thanks for review notes. See below. On 7/26/21 1:13 PM, Olivier Matz wrote: > Hi Andrew, > > Some comments below. > > On Sat, Jul 24, 2021 at 03:33:13PM +0300, Andrew Rybchenko wrote: >> From: Ivan Ilchenko <ivan.ilche...@oktetlabs.ru> >> >> Document valid combinations of input arguments in accordance with >> current implementation in ethdev. >> >> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID") >> Cc: sta...@dpdk.org >> >> Signed-off-by: Ivan Ilchenko <ivan.ilche...@oktetlabs.ru> >> Signed-off-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> >> Reviewed-by: Andy Moreton <amore...@xilinx.com> >> --- >> lib/ethdev/rte_ethdev.h | 32 +++++++++++++++++++------------- >> 1 file changed, 19 insertions(+), 13 deletions(-) >> >> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h >> index d2b27c351f..b14067fe7e 100644 >> --- a/lib/ethdev/rte_ethdev.h >> +++ b/lib/ethdev/rte_ethdev.h >> @@ -2872,13 +2872,17 @@ int rte_eth_xstats_get(uint16_t port_id, struct >> rte_eth_xstat *xstats, >> * @param port_id >> * The port identifier of the Ethernet device. >> * @param xstats_names >> - * An rte_eth_xstat_name array of at least *size* elements to >> - * be filled. If set to NULL, the function returns the required number >> - * of elements. >> + * Array to be filled in with names of requested device statistics. >> + * Must not be NULL if @p ids are specified (not NULL). >> * @param ids >> - * IDs array given by app to retrieve specific statistics >> + * IDs array given by app to retrieve specific statistics. May be NULL to >> + * retrieve names of all available statistics or, if @p xstats_names is >> + * NULL as well, just a number of available statistics. > > double spaces before "just" > > "a number" -> "the number"? Fixed in v5 >> * @param size >> - * The size of the xstats_names array (number of elements). >> + * If @p ids is not NULL, number of elements in the array with requested >> IDs >> + * and number of elements in @p xstats_names to put names in. If @p ids is >> + * NULL, number of elements in @p xstats_names to put all available >> statistics >> + * names in. > > Just a suggestion here, I feel the following description would be clearer: > > Number of elements in @p xstats_names array (if not NULL) and > in @p ids array (if not NULL). I agree that it is better to avoid here details about behaviour of one or another array pointer is NULL. Descriptions of corresponding parameters cover it. Fixed in v5. > > Shouldn't we say that it has to be 0 if both arrays are NULL? Yes, I think it is useful. Will add in v5. > > Also, the order of arguments is not the same in comment and in > the function. I think it can make sense to align the comment > to the prototype. Fixed in v5. > > >> * @return >> * - A positive value lower or equal to size: success. The return value >> * is the number of entries filled in the stats table. > > Not seen in the patch, but right after this line, there is: > > * - A positive value higher than size: error, the given statistics table > * is too small. The return value corresponds to the size that should > * be given to succeed. The entries in the table are not valid and > * shall not be used by the caller. > > I wonder if it shouldn't be slighly reworded to remove 'error'. After > all, passing NULL arrays (and size == 0) is a valid, so the return is > not an error. I agree that it should not be treated as an error. It is just a special case of success when return is partially provided (just a number of stats). Fixed in v5. > >> @@ -2886,7 +2890,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct >> rte_eth_xstat *xstats, >> * is too small. The return value corresponds to the size that should >> * be given to succeed. The entries in the table are not valid and >> * shall not be used by the caller. >> - * - A negative value on error (invalid port id). >> + * - A negative value on error. >> */ >> int >> rte_eth_xstats_get_names_by_id(uint16_t port_id, >> @@ -2899,14 +2903,16 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id, >> * @param port_id >> * The port identifier of the Ethernet device. >> * @param ids >> - * A pointer to an ids array passed by application. This tells which >> - * statistics values function should retrieve. This parameter >> - * can be set to NULL if size is 0. In this case function will retrieve >> - * all available statistics. >> + * IDs array given by app to retrieve specific statistics. May be NULL to >> + * retrieve all available statistics or, if @p values is NULL as well, >> + * just a number of available statistics. >> * @param values >> - * A pointer to a table to be filled with device statistics values. >> + * Array to be filled in with requested device statistics. >> + * Must not be NULL if ids are specified (not NULL). >> * @param size >> - * The size of the ids array (number of elements). >> + * If @p ids is not NULL, number of elements in the array with requested >> IDs >> + * and number of elements in @p values to put statistics in. If @p ids is >> NULL, >> + * number of elements in @p values to put all available statistics in. >> * @return >> * - A positive value lower or equal to size: success. The return value >> * is the number of entries filled in the stats table. >> @@ -2914,7 +2920,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id, >> * is too small. The return value corresponds to the size that should >> * be given to succeed. The entries in the table are not valid and >> * shall not be used by the caller. >> - * - A negative value on error (invalid port id). >> + * - A negative value on error. >> */ > > Some of the comments above also apply here. Done. > >> int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids, >> uint64_t *values, unsigned int size); >> -- >> 2.30.2 >>