Re: [PATCH dpdk v2 1/2] telemetry: add api to register command with private argument

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Oct 03, 2024 at 01:24:41PM +0200, Robin Jarry wrote:
> Add a new rte_telemetry_register_cmd_arg public function to register
> a telemetry endpoint with a callback that takes an additional private
> argument.
> 
> This will be used in the next commit to protect ethdev endpoints with
> a lock.
> 
> Update perform_command() to take a struct callback object copied from
> the list of callbacks and invoke the correct function pointer.
> 
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> ---

I like this solution, and seems a good general addition to telemetry also.
Couple of minor comments inline below.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

>  lib/telemetry/rte_telemetry.h | 46 +++++++++++++++++++++++++++++++++++
>  lib/telemetry/telemetry.c     | 38 +++++++++++++++++++++++------
>  lib/telemetry/version.map     |  3 +++
>  3 files changed, 79 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> index cab9daa6fed6..3fbfda138b16 100644
> --- a/lib/telemetry/rte_telemetry.h
> +++ b/lib/telemetry/rte_telemetry.h
> @@ -336,6 +336,30 @@ rte_tel_data_add_dict_uint_hex(struct rte_tel_data *d, const char *name,
>  typedef int (*telemetry_cb)(const char *cmd, const char *params,
>  		struct rte_tel_data *info);
>  
> +/**
> + * This telemetry callback is used when registering a telemetry command with
> + * rte_telemetry_register_cmd_arg().
> + *
> + * It handles getting and formatting information to be returned to telemetry
> + * when requested.
> + *
> + * @param cmd
> + * The cmd that was requested by the client.
> + * @param params
> + * Contains data required by the callback function.
> + * @param info
> + * The information to be returned to the caller.
> + * @param arg
> + * The opaque value that was passed to rte_telemetry_register_cmd_arg().
> + *

I think we tend to slightly indent the text on the line after the @param,
as in:

 * @param arg
 *    The opaque value...

> + * @return
> + * Length of buffer used on success.
> + * @return
> + * Negative integer on error.
> + */
> +typedef int (*telemetry_arg_cb)(const char *cmd, const char *params,
> +		struct rte_tel_data *info, void *arg);
> +

Not sure about this, but I'd tend to have the arg parameter as second
parameter, to keep the "info" as the final parameter. My suggested order
would be: (cmd, arg, params, info)

>  /**
>   * Used for handling data received over a telemetry socket.
>   *
> @@ -367,6 +391,28 @@ typedef void * (*handler)(void *sock_id);
>  int
>  rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
>  

<snip>