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

["Robin Jarry" <rjarry@redhat.com>]

Bruce Richardson, Oct 03, 2024 at 13:39:
> 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...

I found this weird too but followed the style in the rest of the file. 
I can fix that for a v3.

>
>> + * @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)

I don't have any objections. I'll send a v3.