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 <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...
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.
