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 <rja...@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.richard...@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>
