From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6AF80A0032; Tue, 28 Sep 2021 14:01:06 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5C798410DC; Tue, 28 Sep 2021 14:01:06 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 2FE7540DF6; Tue, 28 Sep 2021 14:01:05 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id B08F37F50B; Tue, 28 Sep 2021 15:01:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru B08F37F50B DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1632830464; bh=mvC3iHLEPN8vgWxDfXhfSIvLfdeA3NmJnW7kVpWoUHc=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=GTUlHcYbZaVPuCj6e3Z3bz2aczh/VDeuYY0pNjeHU49215S5iwnPaZ9b6WfaKKlwn xAXqj6T9zasaIkzHn2f7xuw/D20blmH2t4pA293QYbzfUouoyy/ijXNkLilKV+A720 GzJugNFtaWoRXf7Q4uf9b+dGgyOIPXe3ov6DmBHw= To: Olivier Matz Cc: dev@dpdk.org, Thomas Monjalon , Ferruh Yigit , Kuba Kozak , Ivan Ilchenko , stable@dpdk.org, Andy Moreton References: <20210604144225.287678-1-andrew.rybchenko@oktetlabs.ru> <20210724123314.2970537-1-andrew.rybchenko@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <82862a25-5a81-55e1-f33c-96329c9cacc9@oktetlabs.ru> Date: Tue, 28 Sep 2021 15:01:04 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 1/2] ethdev: fix docs of functions getting xstats by IDs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> >> Document valid combinations of input arguments in accordance with >> current implementation in ethdev. >> >> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID") >> Cc: stable@dpdk.org >> >> Signed-off-by: Ivan Ilchenko >> Signed-off-by: Andrew Rybchenko >> Reviewed-by: Andy Moreton >> --- >> 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 >>