Re: [PATCH v3 03/10] net/hns3: adjust retval when xstats is null of get xstats

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 5/5/22 11:02, Chengwen Feng wrote:
> Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
> to retrieve the required number of elements, but currently hns3 PMD
> returns zero when xstats is NULL.
> 
> This patch adjusts that the return value was the required number of
> elements when stats is NULL.
> 
> Fixes: 8839c5e202f3 ("net/hns3: support device stats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>   drivers/net/hns3/hns3_stats.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
> index 806720faff..4165e22f7f 100644
> --- a/drivers/net/hns3/hns3_stats.c
> +++ b/drivers/net/hns3/hns3_stats.c
> @@ -1020,7 +1020,7 @@ hns3_imissed_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>    * @praram xstats
>    *   A pointer to a table of structure of type *rte_eth_xstat*
>    *   to be filled with device statistics ids and values.
> - *   This parameter can be set to NULL if n is 0.
> + *   If set to NULL, the function returns the required number of elements.
>    * @param n
>    *   The size of the xstats array (number of elements).
>    * @return
> @@ -1041,11 +1041,8 @@ hns3_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>   	int count;
>   	int ret;
>   
> -	if (xstats == NULL)
> -		return 0;
> -

Above lines are OK. ethdev layer should care about it. See
below.

>   	count = hns3_xstats_calc_num(dev);
> -	if ((int)n < count)
> +	if (xstats == NULL || (int)n < count)

but I dislike the change. xstats may be NULL if and only if n
is 0. If so, the extra check for xstats vs NULL is not required
here.

IMHO ethdev must guarantee it. I.e. rte_eth_xstats_get()
implementation in the first patch should be updated to
return -EINVAL if xstats is NULL, but n is not 0.

The major point for me here is to have just one natural key
parameter to make a decision if number of stats should be
returned or we really need to fill xstats in.
n is the parameter. If space is insufficient to return
xstats, return required space. If n is 0, xstats may be NULL,
since we provide no space for values. If we provide some
space for values (n is greater than 0), xstats must be not
NULL.

One more possible improvement in rte_eth_xstats_get()
implementation is to no provide out of xstats array
pointer if n is smaller or equal to number of basic
stats. Of course, space passed to xstats_get will be
zero anyway and xstats pointer should not be used, but
it is still safer to care about it explicitly. I.e. something like

2984 »···»···xcount = (*dev->dev_ops->xstats_get)(dev,
2985 »···»···»···»···     (n > count) ? xstats + count : NULL,
2986 »···»···»···»···     (n > count) ? n - count : 0);

If we guarantee that xstats is not NULL, if n is positive,
extra check for xstats vs NULL will not be required.

>   		return count;
>   
>   	count = 0;