Re: [dpdk-dev] [PATCH v2 06/16] telemetry: add utility functions for creating json

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Apr 08, 2020 at 07:12:57PM +0100, Wiles, Keith wrote:
> 
> 
> > On Apr 8, 2020, at 11:49 AM, Power, Ciara <ciara.power@intel.com> wrote:
> >
> > From: Bruce Richardson <bruce.richardson@intel.com>
> >
> > The functions added in this patch will make it easier for applications
> > to build up correct JSON responses to telemetry requests.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > lib/librte_telemetry/Makefile             |   1 +
> > lib/librte_telemetry/meson.build          |   2 +-
> > lib/librte_telemetry/rte_telemetry.h      |   1 +
> > lib/librte_telemetry/rte_telemetry_json.h | 205 ++++++++++++++++++++++
> > 4 files changed, 208 insertions(+), 1 deletion(-)
> > create mode 100644 lib/librte_telemetry/rte_telemetry_json.h
<snip>
> > +/**
> > + * @internal
> > + *
> > + * Copies a value into a buffer if the buffer has enough available space.
> > + * Nothing written to buffer if an overflow ocurs.
> > + * This function is not for use for values larger than 1k.
> > + *
> > + * @param buf
> > + * Buffer for data to be appended to.
> > + * @param len
> > + * Length of buffer.
> > + * @param format
> > + * Format string.
> > + * @param ...
> > + * Optional arguments that may be required by the format string.
> > + *
> > + * @return
> > + *  Number of characters added to buffer
> > + */
> > +__attribute__((__format__(__printf__, 3, 4)))
> > +static inline int
> > +__json_snprintf(char *buf, const int len, const char *format, ...)
> > +{
> > +char tmp[1024];
> > +va_list ap;
> > +int ret;
> > +
> > +va_start(ap, format);
> > +ret = vsnprintf(tmp, sizeof(tmp), format, ap);
> > +va_end(ap);
> 
> Would strlcpy(buf, tmp, len); reduce the test below? vsnprintf() writes at most size bytes including the ‘\0’ to the buffer. Maybe able to reduce the code to this:
> 
> return strlcpy(buf, tmp, len);// strlcpy returns the total length of buf.
> 
> This means the ret is not really required, unless I missed something.
> 
> > +if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
> > +strcpy(buf, tmp);
> > +return ret;
> > +}
> > +return 0; /* nothing written or modified */
> > +}
> > +
The intent here is that the buffer remains unmodified and the function
returns zero in the case that the printf fails to work. Truncation would
likely leave us with invalid json, so it's all or nothing, which is why we
can't just take the snprintf or strlcpy. [On overflow they still modify the
buffer, which means any closing "]" or "}" in the json would be
overwritten.]

/Bruce