[RFC,1/2] telemetry: support some recursive data objects

  Dict data objects now support uint64_t array data object values.
Only one level of recursion supported.

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 lib/librte_telemetry/rte_telemetry.h          | 27 +++++++++++++++
 .../rte_telemetry_version.map                 |  2 ++
 lib/librte_telemetry/telemetry.c              | 34 +++++++++++++++++++
 lib/librte_telemetry/telemetry_data.c         | 18 ++++++++++
 lib/librte_telemetry/telemetry_data.h         |  3 ++
 lib/librte_telemetry/telemetry_json.h         | 17 ++++++++++
 6 files changed, 101 insertions(+)
  

On Fri, Jun 12, 2020 at 11:53:43AM +0100, Ciara Power wrote:
> Dict data objects now support uint64_t array data object values.
> Only one level of recursion supported.
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>  lib/librte_telemetry/rte_telemetry.h          | 27 +++++++++++++++
>  .../rte_telemetry_version.map                 |  2 ++
>  lib/librte_telemetry/telemetry.c              | 34 +++++++++++++++++++
>  lib/librte_telemetry/telemetry_data.c         | 18 ++++++++++
>  lib/librte_telemetry/telemetry_data.h         |  3 ++
>  lib/librte_telemetry/telemetry_json.h         | 17 ++++++++++
>  6 files changed, 101 insertions(+)
> 
> diff --git a/lib/librte_telemetry/rte_telemetry.h b/lib/librte_telemetry/rte_telemetry.h
> index 2c3c96cf7..dc18c34d0 100644
> --- a/lib/librte_telemetry/rte_telemetry.h
> +++ b/lib/librte_telemetry/rte_telemetry.h
> @@ -44,6 +44,7 @@ enum rte_tel_value_type {
>  	RTE_TEL_STRING_VAL, /** a string value */
>  	RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
>  	RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
> +	RTE_TEL_DATA_VAL,   /** a rte_tel_data pointer value */
>  };
>  
>  /**
> @@ -188,6 +189,22 @@ int
>  rte_tel_data_add_dict_u64(struct rte_tel_data *d,
>  		const char *name, uint64_t val);
>  
> +/**
> + * Add a data object pointer to a dictionary.
> + * The dict must have been started by rte_tel_data_start_dict().
> + *
> + * @param d
> + *   The data structure passed to the callback
> + * @param x
> + *   The data pointer to be returned in the dictionary
> + * @return
> + *   0 on success, negative errno on error
> + */
> +__rte_experimental
> +int
> +rte_tel_data_add_dict_data(struct rte_tel_data *d, const char *name,
> +		struct rte_tel_data *val);
> +
>  /**
>   * This telemetry callback is used when registering a telemetry command.
>   * It handles getting and formatting information to be returned to telemetry
> @@ -253,4 +270,14 @@ int
>  rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
>  		const char **err_str);
>  
> +/**
> + * Get the size of the rte_tel_data struct.
> + *
> + * @return
> + *  size_t of the struct
> + */
> +__rte_experimental
> +size_t
> +rte_tel_get_data_size(void);
> +
Thanks for this work. Thinking about this, the biggest issue I believe is
always going to be the memory management aspect of it. It seems here that
you are providing an API to allow the memory allocation of the additional
object to be done by the callback itself, but that leaves open a problem of
freeing the memory again - how does the telemetry library know what
allocation function was used, so it knows to free it appropriately? This
could be done by function pointer passed in by when adding the data
element, but I think that is clunky.

Perhaps a better approach here might be to instead add APIs for data alloc
and free, so that there is only one way to manage the memory. That would
remove the need for this size function.
  

On Fri, Jun 12, 2020 at 11:53:43AM +0100, Ciara Power wrote:
> Dict data objects now support uint64_t array data object values.
> Only one level of recursion supported.
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>  lib/librte_telemetry/rte_telemetry.h          | 27 +++++++++++++++
>  .../rte_telemetry_version.map                 |  2 ++
>  lib/librte_telemetry/telemetry.c              | 34 +++++++++++++++++++
>  lib/librte_telemetry/telemetry_data.c         | 18 ++++++++++
>  lib/librte_telemetry/telemetry_data.h         |  3 ++
>  lib/librte_telemetry/telemetry_json.h         | 17 ++++++++++
>  6 files changed, 101 insertions(+)
> 
> diff --git a/lib/librte_telemetry/rte_telemetry.h b/lib/librte_telemetry/rte_telemetry.h
> index 2c3c96cf7..dc18c34d0 100644
> --- a/lib/librte_telemetry/rte_telemetry.h
> +++ b/lib/librte_telemetry/rte_telemetry.h
> @@ -44,6 +44,7 @@ enum rte_tel_value_type {
>  	RTE_TEL_STRING_VAL, /** a string value */
>  	RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
>  	RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
> +	RTE_TEL_DATA_VAL,   /** a rte_tel_data pointer value */

Are there plans to allow arrays of this type since it's added to the enum.
Might be worth adding in v2.

>  };
>  
>  /**
> @@ -188,6 +189,22 @@ int
>  rte_tel_data_add_dict_u64(struct rte_tel_data *d,
>  		const char *name, uint64_t val);
>  
> +/**
> + * Add a data object pointer to a dictionary.
> + * The dict must have been started by rte_tel_data_start_dict().
> + *
> + * @param d
> + *   The data structure passed to the callback
> + * @param x
> + *   The data pointer to be returned in the dictionary
> + * @return
> + *   0 on success, negative errno on error
> + */
> +__rte_experimental
> +int
> +rte_tel_data_add_dict_data(struct rte_tel_data *d, const char *name,
> +		struct rte_tel_data *val);
> +
>  /**
>   * This telemetry callback is used when registering a telemetry command.
>   * It handles getting and formatting information to be returned to telemetry
> @@ -253,4 +270,14 @@ int
>  rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
>  		const char **err_str);
>  
> +/**
> + * Get the size of the rte_tel_data struct.
> + *
> + * @return
> + *  size_t of the struct
> + */
> +__rte_experimental
> +size_t
> +rte_tel_get_data_size(void);
> +
>  #endif
> diff --git a/lib/librte_telemetry/rte_telemetry_version.map b/lib/librte_telemetry/rte_telemetry_version.map
> index 86433c21d..c5425eff6 100644
> --- a/lib/librte_telemetry/rte_telemetry_version.map
> +++ b/lib/librte_telemetry/rte_telemetry_version.map
> @@ -4,6 +4,7 @@ EXPERIMENTAL {
>  	rte_tel_data_add_array_int;
>  	rte_tel_data_add_array_string;
>  	rte_tel_data_add_array_u64;
> +	rte_tel_data_add_dict_data;
>  	rte_tel_data_add_dict_int;
>  	rte_tel_data_add_dict_string;
>  	rte_tel_data_add_dict_u64;
> @@ -13,6 +14,7 @@ EXPERIMENTAL {
>  	rte_telemetry_init;
>  	rte_telemetry_legacy_register;
>  	rte_telemetry_register_cmd;
> +	rte_tel_get_data_size;
>  
>  	local: *;
>  };
> diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c
> index 7b6f8a79e..b3a5f4296 100644
> --- a/lib/librte_telemetry/telemetry.c
> +++ b/lib/librte_telemetry/telemetry.c
> @@ -47,6 +47,12 @@ static int num_callbacks; /* How many commands are registered */
>  /* Used when accessing or modifying list of command callbacks */
>  static rte_spinlock_t callback_sl = RTE_SPINLOCK_INITIALIZER;
>  
> +size_t
> +rte_tel_get_data_size(void)
> +{
> +	return DATA_STRUCT_SIZE;
> +}
> +
>  int
>  rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
>  {
> @@ -120,6 +126,23 @@ command_help(const char *cmd __rte_unused, const char *params,
>  	return 0;
>  }
>  
> +static int
> +recursive_data_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> +{
> +	size_t used = 0;
> +	unsigned int i;
> +
> +	if (d->type != RTE_TEL_ARRAY_U64)
> +		return snprintf(out_buf, buf_len, "null");
> +
> +	used = rte_tel_json_empty_array(out_buf, buf_len, 0);
> +	for (i = 0; i < d->data_len; i++)
> +		used = rte_tel_json_add_array_u64(out_buf,
> +				buf_len, used,
> +				d->data.array[i].u64val);
> +	return used;
> +}
> +
>  static void
>  output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  {
> @@ -166,6 +189,17 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  						buf_len, used,
>  						v->name, v->value.u64val);
>  				break;
> +			case RTE_TEL_DATA_VAL:
> +			{
> +				char temp[buf_len];
> +				if (recursive_data_json(v->value.dataval,
> +						temp, buf_len) != 0)
> +					used = rte_tel_json_add_obj_json(
> +							cb_data_buf,
> +							buf_len, used,
> +							v->name, temp);
> +				free(v->value.dataval);
> +			}
>  			}
>  		}
>  		used += prefix_used;
> diff --git a/lib/librte_telemetry/telemetry_data.c b/lib/librte_telemetry/telemetry_data.c
> index f424bbd48..46ce7f9b0 100644
> --- a/lib/librte_telemetry/telemetry_data.c
> +++ b/lib/librte_telemetry/telemetry_data.c
> @@ -128,3 +128,21 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
>  	const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
>  	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
>  }
> +
> +int
> +rte_tel_data_add_dict_data(struct rte_tel_data *d, const char *name,
> +		struct rte_tel_data *val)
> +{
> +	struct tel_dict_entry *e = &d->data.dict[d->data_len];
> +
> +	if (d->type != RTE_TEL_DICT)
> +		return -EINVAL;
> +	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> +		return -ENOSPC;
> +
> +	d->data_len++;
> +	e->type = RTE_TEL_DATA_VAL;
> +	e->value.dataval = val;

I think we need some restrictions here and return error if not met.
1) I believe we only plan to support one level of recursion, so therefore
we need to check that the added element does not have any other data values
already hanging off it.
2) Right now this only supports arrays of numbers in the added data, so
that needs to be checked.

> +	const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> +	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> +}
> diff --git a/lib/librte_telemetry/telemetry_data.h b/lib/librte_telemetry/telemetry_data.h
> index ff3a371a3..226d961e0 100644
> --- a/lib/librte_telemetry/telemetry_data.h
> +++ b/lib/librte_telemetry/telemetry_data.h
> @@ -8,6 +8,8 @@
>  #include <inttypes.h>
>  #include "rte_telemetry.h"
>  
> +#define DATA_STRUCT_SIZE sizeof(struct rte_tel_data)
> +
>  enum tel_container_types {
>  	RTE_TEL_NULL,	      /** null, used as error value */
>  	RTE_TEL_STRING,	      /** basic string type, no included data */
> @@ -25,6 +27,7 @@ union tel_value {
>  	char sval[RTE_TEL_MAX_STRING_LEN];
>  	int ival;
>  	uint64_t u64val;
> +	struct rte_tel_data *dataval;
>  };
>  
>  struct tel_dict_entry {
> diff --git a/lib/librte_telemetry/telemetry_json.h b/lib/librte_telemetry/telemetry_json.h
> index a2ce4899e..415cfe7c6 100644
> --- a/lib/librte_telemetry/telemetry_json.h
> +++ b/lib/librte_telemetry/telemetry_json.h
> @@ -155,4 +155,21 @@ rte_tel_json_add_obj_str(char *buf, const int len, const int used,
>  	return ret == 0 ? used : end + ret;
>  }
>  
> +/**
> + * Add a new element with raw JSON value to the JSON object stored in the
> + * provided buffer.
> + */
> +static inline int
> +rte_tel_json_add_obj_json(char *buf, const int len, const int used,
> +		const char *name, const char *val)
> +{
> +	int ret, end = used - 1;
> +	if (used <= 2) /* assume empty, since minimum is '{}' */
> +		return __json_snprintf(buf, len, "{\"%s\":%s}", name, val);
> +
> +	ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
> +			name, val);
> +	return ret == 0 ? used : end + ret;
> +}
> +
>  #endif /*_RTE_TELEMETRY_JSON_H_*/
> -- 
> 2.17.1
>
  

On Fri, Jun 12, 2020 at 02:07:06PM +0100, Bruce Richardson wrote:
> On Fri, Jun 12, 2020 at 11:53:43AM +0100, Ciara Power wrote:
> > Dict data objects now support uint64_t array data object values.
> > Only one level of recursion supported.
> > 
> > Signed-off-by: Ciara Power <ciara.power@intel.com>
> > ---
<snip>
> >  static void
> >  output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >  {
> > @@ -166,6 +189,17 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >  						buf_len, used,
> >  						v->name, v->value.u64val);
> >  				break;
> > +			case RTE_TEL_DATA_VAL:
> > +			{
> > +				char temp[buf_len];
> > +				if (recursive_data_json(v->value.dataval,
> > +						temp, buf_len) != 0)
> > +					used = rte_tel_json_add_obj_json(
> > +							cb_data_buf,
> > +							buf_len, used,
> > +							v->name, temp);
> > +				free(v->value.dataval);

Are there cases where we want to preserve the structure across calls rather
than doing an alloc and free each time?