[v2] telemetry: fix json output buffer size

  Fix json output buffer size for a single largest value.

Fixes: 52af6ccb2b39 ("telemetry: add utility functions for creating JSON")

v2:
 - split from series 18768 ("cnxk: enable telemetry endpoints").

Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
---
 lib/telemetry/telemetry_json.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
  

Hi Gowrishankar,


>-----Original Message-----
>From: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
>Sent: Tuesday 21 September 2021 12:03
>To: dev@dpdk.org
>Cc: Power, Ciara <ciara.power@intel.com>; Gowrishankar Muthukrishnan
><gmuthukrishn@marvell.com>
>Subject: [v2] telemetry: fix json output buffer size
>
>Fix json output buffer size for a single largest value.
>
>Fixes: 52af6ccb2b39 ("telemetry: add utility functions for creating JSON")
>
>v2:
> - split from series 18768 ("cnxk: enable telemetry endpoints").
>
>Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
>---
> lib/telemetry/telemetry_json.h | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
>index ad270b9b30..ba2fde34cb 100644
>--- a/lib/telemetry/telemetry_json.h
>+++ b/lib/telemetry/telemetry_json.h
>@@ -9,6 +9,7 @@
> #include <stdarg.h>
> #include <stdio.h>
> #include <rte_common.h>
>+#include <rte_telemetry.h>
>
> /**
>  * @file
>@@ -23,13 +24,15 @@
>  * @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.
>+ * Size of buffer is (single largest value - 6), where at least 6 chars
>+ * would have been used for creating json dict i.e '{"x": ... }'.
>+ * This function is not for use for values larger than this buffer size.
>  */
> __rte_format_printf(3, 4)
> static inline int
> __json_snprintf(char *buf, const int len, const char *format, ...)  {
>-	char tmp[1024];
>+	char tmp[RTE_TEL_MAX_SINGLE_STRING_LEN - 6];
> 	va_list ap;
> 	int ret;
>
>--
>2.25.1

I am not sure about why we would want this to allow for "RTE_TEL_MAX_SINGLE_STRING_LEN - 6".
The RTE_TEL_MAX_SINGLE_STRING_LEN is used to represent the max size of a singular string value e.g. the response to client being   {"<cmd>" : "<string value here up to max size in length>" }

I wonder could we use the "len" parameter in some way here, that would be the available space to be filled of the "buf" being passed in, allowing the function to copy in the maximum amount to fill the buffer.

Thanks,
Ciara
  

Hi Ciara,
> I am not sure about why we would want this to allow for
> "RTE_TEL_MAX_SINGLE_STRING_LEN - 6".
> The RTE_TEL_MAX_SINGLE_STRING_LEN is used to represent the max size of a
> singular string value e.g. the response to client being   {"<cmd>" : "<string value
> here up to max size in length>" }
> 
> I wonder could we use the "len" parameter in some way here, that would be the
> available space to be filled of the "buf" being passed in, allowing the function to
> copy in the maximum amount to fill the buffer.

Got it. Yeah, "len" is actual available space. I ll send next version based on this.

Also, I propose if we can have platform defined upper limits (esp MAX_CMD_LEN,
MAX_SINGLE_STRING_LEN etc) so that, we need not revisit lib/telemetry for
platform needs (and I don't think one size fits all platform, may be excess too).
Thoughts ?

Thanks,
Gowrishankar

> 
> Thanks,
> Ciara
  

Hi Gowrishankar,

>-----Original Message-----
>From: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
>Sent: Thursday 23 September 2021 06:53
>To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
>Cc: Richardson, Bruce <bruce.richardson@intel.com>
>Subject: RE: [v2] telemetry: fix json output buffer size
>
>Hi Ciara,
>> I am not sure about why we would want this to allow for
>> "RTE_TEL_MAX_SINGLE_STRING_LEN - 6".
>> The RTE_TEL_MAX_SINGLE_STRING_LEN is used to represent the max size
>of a
>> singular string value e.g. the response to client being   {"<cmd>" : "<string
>value
>> here up to max size in length>" }
>>
>> I wonder could we use the "len" parameter in some way here, that would
>> be the available space to be filled of the "buf" being passed in,
>> allowing the function to copy in the maximum amount to fill the buffer.
>
>Got it. Yeah, "len" is actual available space. I ll send next version based on this.
>
>Also, I propose if we can have platform defined upper limits (esp
>MAX_CMD_LEN, MAX_SINGLE_STRING_LEN etc) so that, we need not revisit
>lib/telemetry for platform needs (and I don't think one size fits all platform,
>may be excess too).
>Thoughts ?

I am not sure why it is needed to have limits defined per platform - can you explain further about why it is necessary?

Thanks,
Ciara

>
>Thanks,
>Gowrishankar
>
>>
>> Thanks,
>> Ciara
  

> >Also, I propose if we can have platform defined upper limits (esp
> >MAX_CMD_LEN, MAX_SINGLE_STRING_LEN etc) so that, we need not revisit
> >lib/telemetry for platform needs (and I don't think one size fits all
> >platform, may be excess too).
> >Thoughts ?
> 
> I am not sure why it is needed to have limits defined per platform - can you
> explain further about why it is necessary?
> 

Mainly, for the endpoint in driver. In case, if the endpoint data is bigger than MAX_SINGLE_STRING_LEN
at the worst case, endpoint will not work correctly.

Thanks,
Gowrishankar
  

Hi Gowrishankar,

>-----Original Message-----
>From: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
>Sent: Thursday 30 September 2021 10:01
>To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
>Cc: Richardson, Bruce <bruce.richardson@intel.com>
>Subject: RE: [v2] telemetry: fix json output buffer size
>
>> >Also, I propose if we can have platform defined upper limits (esp
>> >MAX_CMD_LEN, MAX_SINGLE_STRING_LEN etc) so that, we need not revisit
>> >lib/telemetry for platform needs (and I don't think one size fits all
>> >platform, may be excess too).
>> >Thoughts ?
>>
>> I am not sure why it is needed to have limits defined per platform -
>> can you explain further about why it is necessary?
>>
>
>Mainly, for the endpoint in driver. In case, if the endpoint data is bigger than
>MAX_SINGLE_STRING_LEN at the worst case, endpoint will not work correctly.
>

The MAX_SINGLE_STRING_LEN is currently 8192 - if a value is turning out to be bigger than that, maybe we should be looking at why the string is that long and if it needs to be - i.e. could it be broken down to be more consumable rather than modifying the lib to accept an extremely long string.
I don't see an issue with increasing the MAX_CMD_LEN a little in the lib - although it is currently 56, it probably doesn't need to be a huge increase, commands are easier used when concise.

Not sure having platform specific values here is necessary. 

Thanks,
Ciara

>Thanks,
>Gowrishankar