RE: [PATCH] telemetry: support boolean type

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 19 October 2022 15.48
> 
> On Wed, Oct 19, 2022 at 09:37:02AM +0200, David Marchand wrote:
> > Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> 
> This patch looks pretty good to me. Some very small comments inline
> below.
> One thing I notice is that we are not supporting booleans except as
> part of
> an array or dictionary. Is it likely that we will ever want to have a
> telemetry command that just returns true/false alone? Don't see that
> being
> necessary just yet, so:
> 
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[...]

> > +/* Appends a boolean into the JSON array in the provided buffer. */
> > +static inline int
> > +rte_tel_json_add_array_bool(char *buf, const int len, const int
> used,
> > +		bool val)
> > +{
> > +	int ret, end = used - 1; /* strip off final delimiter */
> > +	if (used <= 2) /* assume empty, since minimum is '[]' */
> > +		return __json_snprintf(buf, len, "[%s]",
> > +				val ? "true" : "false");
> > +
> > +	ret = __json_snprintf(buf + end, len - end, ",%s]",
> > +			val ? "true" : "false");
> 
> Wonder if it's worthwhile doing a macro for this conditional, since the
> same ternary-operator snippet appears 4 times in this code.

A macro will probably degrade source code readability. Please keep as is.

I considered moving the conditional inside the format string to get rid of the %s. But there is no performance requirement, and it is the exception that it can be done for Booleans (but not integers and other types). So I concluded that the current form is good.