[PATCH 0/2] improve code portability

[PATCH 0/2] improve code portability

[Tyler Retzlaff]

Improve portability of telemetry code to allow it to be compiled by msvc
unconditionally.

Remove use of VLA and instead dynamically allocate. MSVC will never
implement VLAs due to misuse / security concerns. 

Remove use of ranged based initializer (a gcc extension) instead just
explicitly initialize individual array elements.

Tyler Retzlaff (2):
  telemetry: use malloc instead of variable length array
  telemetry: use portable syntax to initialize array

 lib/telemetry/telemetry_data.c | 20 ++++++++++++++------
 lib/telemetry/telemetry_json.h | 32 +++++++++++++++++++++++---------
 2 files changed, 37 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] telemetry: use malloc instead of variable length array

[Tyler Retzlaff]

Replace use of variable length array optional standard feature to
improve portability.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/telemetry/telemetry_json.h | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 744bbfe..c375e97 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -30,18 +30,23 @@
 static inline int
 __json_snprintf(char *buf, const int len, const char *format, ...)
 {
-	char tmp[len];
+	char *tmp = malloc(len);
 	va_list ap;
-	int ret;
+	int ret = 0;
+
+	if (tmp == NULL)
+		return ret;
 
 	va_start(ap, format);
 	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
 	va_end(ap);
 	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
 		strcpy(buf, tmp);
-		return ret;
 	}
-	return 0; /* nothing written or modified */
+
+	free(tmp);
+
+	return ret;
 }
 
 static const char control_chars[0x20] = {
@@ -60,13 +65,17 @@
 static inline int
 __json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
 {
-	char tmp[len];
 	int tmpidx = 0;
+	int ret = 0;
+	char *tmp = malloc(len);
+
+	if (tmp == NULL)
+		return ret;
 
 	while (*prefix != '\0' && tmpidx < len)
 		tmp[tmpidx++] = *prefix++;
 	if (tmpidx >= len)
-		return 0;
+		goto cleanup;
 
 	while (*str != '\0') {
 		if (*str < (int)RTE_DIM(control_chars)) {
@@ -85,19 +94,24 @@
 		 * escaped character like "\n" without overflowing
 		 */
 		if (tmpidx > len - 2)
-			return 0;
+			goto cleanup;
 		str++;
 	}
 
 	while (*suffix != '\0' && tmpidx < len)
 		tmp[tmpidx++] = *suffix++;
 	if (tmpidx >= len)
-		return 0;
+		goto cleanup;
 
 	tmp[tmpidx] = '\0';
 
 	strcpy(buf, tmp);
-	return tmpidx;
+	ret = tmpidx;
+
+cleanup:
+	free(tmp);
+
+	return ret;
 }
 
 /* Copies an empty array into the provided buffer. */
-- 
1.8.3.1

[PATCH 2/2] telemetry: use portable syntax to initialize array

[Tyler Retzlaff]

Use of ranges in designated initialization are a non-standard gcc
extension. Manually expand the ranges used in designated initialization
to eliminate the use of non-standard features.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/telemetry/telemetry_data.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 2bac2de..87e5f18 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -152,12 +152,20 @@
 static bool
 valid_name(const char *name)
 {
-	char allowed[128] = {
-			['0' ... '9'] = 1,
-			['A' ... 'Z'] = 1,
-			['a' ... 'z'] = 1,
-			['_'] = 1,
-			['/'] = 1,
+	static const char allowed[128] = {
+		['0'] = 1, ['1'] = 1, ['2'] = 1, ['3'] = 1, ['4'] = 1,
+		['5'] = 1, ['6'] = 1, ['7'] = 1, ['8'] = 1, ['9'] = 1,
+		['A'] = 1, ['B'] = 1, ['C'] = 1, ['D'] = 1, ['E'] = 1,
+		['F'] = 1, ['G'] = 1, ['H'] = 1, ['I'] = 1, ['J'] = 1,
+		['K'] = 1, ['L'] = 1, ['M'] = 1, ['N'] = 1, ['O'] = 1,
+		['P'] = 1, ['Q'] = 1, ['R'] = 1, ['S'] = 1, ['T'] = 1,
+		['U'] = 1, ['V'] = 1, ['W'] = 1, ['X'] = 1, ['Y'] = 1,
+		['Z'] = 1, ['a'] = 1, ['b'] = 1, ['c'] = 1, ['d'] = 1,
+		['e'] = 1, ['f'] = 1, ['g'] = 1, ['h'] = 1, ['i'] = 1,
+		['j'] = 1, ['k'] = 1, ['l'] = 1, ['m'] = 1, ['n'] = 1,
+		['o'] = 1, ['p'] = 1, ['q'] = 1, ['r'] = 1, ['s'] = 1,
+		['t'] = 1, ['u'] = 1, ['v'] = 1, ['w'] = 1, ['x'] = 1,
+		['y'] = 1, ['z'] = 1, ['_'] = 1, ['/'] = 1,
 	};
 	while (*name != '\0') {
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
-- 
1.8.3.1

Re: [PATCH 0/2] improve code portability

[Bruce Richardson]

On Mon, Apr 03, 2023 at 09:30:22AM -0700, Tyler Retzlaff wrote:
> Improve portability of telemetry code to allow it to be compiled by msvc
> unconditionally.
> 
> Remove use of VLA and instead dynamically allocate. MSVC will never
> implement VLAs due to misuse / security concerns. 
> 
> Remove use of ranged based initializer (a gcc extension) instead just
> explicitly initialize individual array elements.
> 
> Tyler Retzlaff (2):
>   telemetry: use malloc instead of variable length array
>   telemetry: use portable syntax to initialize array
> 
Is this worth doing, given that DPDK telemetry uses a unix domain socket
for it's connectivity, which would not be available on windows anyway?
I don't particularly like these patches as:
* The removal of the VLAs means we will potentially be doing a *lot* of
  malloc and free calls inside the telemetry code. It may not be a data
  path or particularly performance critical, but I know for things like CPU
  busyness, users may want to call telemetry functions hundreds (or
  potentially thousands) of times a second. It also makes the code slightly
  harder to read, and introduces the possibility of us having memory leaks.
* The second patch just makes the code uglier. True, it's non-standard, but
  it really does make the code a whole lot more readable and managable. If
  we need to make this standards-conforming, then I think we need to drop
  the "const", and do runtime init of this array with loops for the ranges.

All that said, if we do have a path to get telemetry working on windows, I
think we can work together to get a suitable patchset in for it.

/Bruce

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Tyler Retzlaff]

On Mon, Apr 03, 2023 at 09:30:23AM -0700, Tyler Retzlaff wrote:
> Replace use of variable length array optional standard feature to
> improve portability.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/telemetry/telemetry_json.h | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> index 744bbfe..c375e97 100644
> --- a/lib/telemetry/telemetry_json.h
> +++ b/lib/telemetry/telemetry_json.h
> @@ -30,18 +30,23 @@
>  static inline int
>  __json_snprintf(char *buf, const int len, const char *format, ...)
>  {
> -	char tmp[len];
> +	char *tmp = malloc(len);
>  	va_list ap;
> -	int ret;
> +	int ret = 0;
> +
> +	if (tmp == NULL)
> +		return ret;
>  
>  	va_start(ap, format);
>  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);

I mistakenly pushed an old rev, i'll fix this.

sorry for the noise.

Re: [PATCH 0/2] improve code portability

[Tyler Retzlaff]

On Mon, Apr 03, 2023 at 06:04:08PM +0100, Bruce Richardson wrote:
> On Mon, Apr 03, 2023 at 09:30:22AM -0700, Tyler Retzlaff wrote:
> > Improve portability of telemetry code to allow it to be compiled by msvc
> > unconditionally.
> > 
> > Remove use of VLA and instead dynamically allocate. MSVC will never
> > implement VLAs due to misuse / security concerns. 
> > 
> > Remove use of ranged based initializer (a gcc extension) instead just
> > explicitly initialize individual array elements.
> > 
> > Tyler Retzlaff (2):
> >   telemetry: use malloc instead of variable length array
> >   telemetry: use portable syntax to initialize array
> > 

Bruce I was hoping to solicit your response specifically on this series so
thanks.

> Is this worth doing, given that DPDK telemetry uses a unix domain socket
> for it's connectivity, which would not be available on windows anyway?
> I don't particularly like these patches as:
> * The removal of the VLAs means we will potentially be doing a *lot* of
>   malloc and free calls inside the telemetry code. It may not be a data
>   path or particularly performance critical, but I know for things like CPU
>   busyness, users may want to call telemetry functions hundreds (or
>   potentially thousands) of times a second. It also makes the code slightly
>   harder to read, and introduces the possibility of us having memory leaks.

Yeah, malloc/free at high frequency is a bit ugly we could use alloca but I
think equally as unsafe as VLAs.

Is the preference here to just not compile any of this code for Windows
regardless of compiler? If that is preferred I can look at refactoring
to do that.

Another option is I could use conditionally compiled code narrowly here
using a Windows only API malloca which is vaguely safer?

> * The second patch just makes the code uglier. True, it's non-standard, but
>   it really does make the code a whole lot more readable and managable. If
>   we need to make this standards-conforming, then I think we need to drop
>   the "const", and do runtime init of this array with loops for the ranges.

I considered this. I thought the preference was to preserve const but
if initialization in a loop is preferrable i'll do that instead.

> 
> All that said, if we do have a path to get telemetry working on windows, I
> think we can work together to get a suitable patchset in for it.

It's unfortunate that EAL depends on telemetry but it doesn't work on
Windows (right now). Telemetry is unfortunately not of much value if the
code doesn't build and run so thus a lower priority.

What I can ask for here (if the community accepts) is getting the code
to compile, that's what unblocks progress for now and we can re-visit
making telemetry work properly ~later.

Thanks

> 
> /Bruce

[PATCH v2] improve code portability

[Tyler Retzlaff]

Improve portability of telemetry code to allow it to be compiled by msvc
unconditionally.

Remove use of ranged based initializer (a gcc extension). Instead initialize
using for loops over [0..9] [A..Z] [a..z] [_/] on first call.

v2:
  * Initialize valid character array using for loop instead of explicit
    initialization of each element.
  * Drop patch removing VLAs, in a separate series a conditionally compiled
    change will be introduced.

Tyler Retzlaff (1):
  telemetry: use portable syntax to initialize array

 lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[PATCH v2] telemetry: use portable syntax to initialize array

[Tyler Retzlaff]

Use of ranges in designated initialization are a non-standard gcc
extension. Use loops to initialize permitted characters on first use.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 2bac2de..d845186 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -152,13 +152,21 @@
 static bool
 valid_name(const char *name)
 {
-	char allowed[128] = {
-			['0' ... '9'] = 1,
-			['A' ... 'Z'] = 1,
-			['a' ... 'z'] = 1,
-			['_'] = 1,
-			['/'] = 1,
-	};
+	static bool initialized;
+	static char allowed[128];
+
+	if (!initialized) {
+		int index;
+		for (index = '0'; index <= '9'; index++)
+			allowed[index] = 1;
+		for (index = 'A'; index <= 'Z'; index++)
+			allowed[index] = 1;
+		for (index = 'a'; index <= 'Z'; index++)
+			allowed[index] = 1;
+		allowed[(int)'_'] = allowed[(int)'/'] = 1;
+		initialized = true;
+	}
+
 	while (*name != '\0') {
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
 			return false;
-- 
1.8.3.1

[PATCH v3] improve code portability

[Tyler Retzlaff]

Improve portability of telemetry code to allow it to be compiled by msvc
unconditionally.

Remove use of ranged based initializer (a gcc extension). Instead initialize
using for loops over [0..9] [A..Z] [a..z] [_/] on first call.

v3:
  * Move int index declaration to top of function
  * Fix typo 'Z' -> 'z' in last for loop
v2:
  * Initialize valid character array using for loop instead of explicit
    initialization of each element.
  * Drop patch removing VLAs, in a separate series a conditionally compiled
    change will be introduced.

Tyler Retzlaff (1):
  telemetry: use portable syntax to initialize array

 lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[PATCH v3] telemetry: use portable syntax to initialize array

[Tyler Retzlaff]

Use of ranges in designated initialization are a non-standard gcc
extension. Use loops to initialize permitted characters on first use.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 2bac2de..562b387 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -152,13 +152,21 @@
 static bool
 valid_name(const char *name)
 {
-	char allowed[128] = {
-			['0' ... '9'] = 1,
-			['A' ... 'Z'] = 1,
-			['a' ... 'z'] = 1,
-			['_'] = 1,
-			['/'] = 1,
-	};
+	int index;
+	static bool initialized;
+	static char allowed[128];
+
+	if (!initialized) {
+		for (index = '0'; index <= '9'; index++)
+			allowed[index] = 1;
+		for (index = 'A'; index <= 'Z'; index++)
+			allowed[index] = 1;
+		for (index = 'a'; index <= 'z'; index++)
+			allowed[index] = 1;
+		allowed[(int)'_'] = allowed[(int)'/'] = 1;
+		initialized = true;
+	}
+
 	while (*name != '\0') {
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
 			return false;
-- 
1.8.3.1

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Stephen Hemminger]

On Mon,  3 Apr 2023 09:30:23 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

>  __json_snprintf(char *buf, const int len, const char *format, ...)
>  {
> -	char tmp[len];
> +	char *tmp = malloc(len);
>  	va_list ap;
> -	int ret;
> +	int ret = 0;
> +
> +	if (tmp == NULL)
> +		return ret;
>  
>  	va_start(ap, format);
>  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
>  	va_end(ap);
>  	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
>  		strcpy(buf, tmp);
> -		return ret;
>  	}
> -	return 0; /* nothing written or modified */
> +
> +	free(tmp);
> +
> +	return ret;
>  }

Not sure why it needs a tmp buffer anyway?

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Tyler Retzlaff]

On Mon, Apr 03, 2023 at 01:19:12PM -0700, Stephen Hemminger wrote:
> On Mon,  3 Apr 2023 09:30:23 -0700
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> >  __json_snprintf(char *buf, const int len, const char *format, ...)
> >  {
> > -	char tmp[len];
> > +	char *tmp = malloc(len);
> >  	va_list ap;
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	if (tmp == NULL)
> > +		return ret;
> >  
> >  	va_start(ap, format);
> >  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
> >  	va_end(ap);
> >  	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
also this seems redundant. when is len != sizeof(tmp) here?

> >  		strcpy(buf, tmp);
> > -		return ret;
> >  	}
> > -	return 0; /* nothing written or modified */
> > +
> > +	free(tmp);
> > +
> > +	return ret;
> >  }
> 
> Not sure why it needs a tmp buffer anyway?

yeah, there a are a few question marks in this code. i've removed this
patch from the series for now. v3 doesn't touch this file anymore.

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Bruce Richardson]

On Mon, Apr 03, 2023 at 01:19:12PM -0700, Stephen Hemminger wrote:
> On Mon,  3 Apr 2023 09:30:23 -0700
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> >  __json_snprintf(char *buf, const int len, const char *format, ...)
> >  {
> > -	char tmp[len];
> > +	char *tmp = malloc(len);
> >  	va_list ap;
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	if (tmp == NULL)
> > +		return ret;
> >  
> >  	va_start(ap, format);
> >  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
> >  	va_end(ap);
> >  	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
> >  		strcpy(buf, tmp);
> > -		return ret;
> >  	}
> > -	return 0; /* nothing written or modified */
> > +
> > +	free(tmp);
> > +
> > +	return ret;
> >  }
> 
> Not sure why it needs a tmp buffer anyway?

The temporary buffer is to ensure that in the case that the data doesn't
fit in the buffer, the buffer remains unmodified. The reason for this is
that when building up the json response we always have a valid json string.

For example, suppose we are preparing a response with an array of two
strings. After the first string has been processed, the output buffer
contains: '["string1"]'. When json_snprintf is being called to add string2,
there are a couple of things to note:
* the text to be inserted will be put not at the end of the string, but
  before the closing "]".
* the actual text to be inserted will be ',"string2"]', so ensuring that
  the final buffer is valid.
However, the error case is problematic. While we can catch the case where
the string to be inserted overflows/has been truncated, doing a regular
snprintf means that our output buffer could contain invalid json, as our
end-terminator would have been overwritten, e.g. '["string1","string2'
To guarantee the output from telemetry is always valid json, even in case
of truncation, we use a temporary buffer to do the write initially, and if
it doesn't get truncated, we then copy that to the final buffer.

That's the logic for this temporary buffer. Now, thinking about it
yesterday evening, there are other ways in which we can do this, which can
avoid this temporary buffer.
1. We can do the initial snprintf to an empty buffer to get the length that
   way. This will still be slower, as it means that we need to do printf
   processing twice rather than using memcpy to copy the result. However, it's
   probably less overhead than malloc and free.
2. AFAIK, the normal case for this function being called is with a single
   terminator at the end of the string. We can take advantage of that, by
   checking if the '\0' just one character into the string we are printing,
   and, if so, to store that once character. If we have a snprintf error
   leading to truncation, it then allows us to restore the original string.

My suggestion is to use a combination of these methods. In json_snprintf
check if the input buffer is empty or has only one character in it, and use
method #2 if so. If that's not the case, then fallback to method #1 and do
a double snprintf.

Make sense? Any other suggestions?

/Bruce

Re: [PATCH v3] telemetry: use portable syntax to initialize array

[Bruce Richardson]

On Mon, Apr 03, 2023 at 11:59:25AM -0700, Tyler Retzlaff wrote:
> Use of ranges in designated initialization are a non-standard gcc
> extension. Use loops to initialize permitted characters on first use.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 2bac2de..562b387 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -152,13 +152,21 @@
>  static bool
>  valid_name(const char *name)
>  {
> -	char allowed[128] = {
> -			['0' ... '9'] = 1,
> -			['A' ... 'Z'] = 1,
> -			['a' ... 'z'] = 1,
> -			['_'] = 1,
> -			['/'] = 1,
> -	};
> +	int index;

My preference would be to limit the scope of index to the if block, but ok
to keep as here.

[In fact, when we switch to C11, I'd love to see the coding standards
relaxed to allow loop variable definition inside the for statement itself]

> +	static bool initialized;
> +	static char allowed[128];
> +
> +	if (!initialized) {
> +		for (index = '0'; index <= '9'; index++)
> +			allowed[index] = 1;
> +		for (index = 'A'; index <= 'Z'; index++)
> +			allowed[index] = 1;
> +		for (index = 'a'; index <= 'z'; index++)
> +			allowed[index] = 1;
> +		allowed[(int)'_'] = allowed[(int)'/'] = 1;
> +		initialized = true;
> +	}
> +
>  	while (*name != '\0') {
>  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
>  			return false;
> -- 
> 1.8.3.1
> 

RE: [PATCH v3] telemetry: use portable syntax to initialize array

[Konstantin Ananyev]

> -----Original Message-----
> From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Sent: Monday, April 3, 2023 7:59 PM
> To: dev@dpdk.org
> Cc: ciara.power@intel.com; bruce.richardson@intel.com; david.marchand@redhat.com; thomas@monjalon.net; Tyler Retzlaff
> <roretzla@linux.microsoft.com>
> Subject: [PATCH v3] telemetry: use portable syntax to initialize array
> 
> Use of ranges in designated initialization are a non-standard gcc
> extension. Use loops to initialize permitted characters on first use.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 2bac2de..562b387 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -152,13 +152,21 @@
>  static bool
>  valid_name(const char *name)
>  {
> -	char allowed[128] = {
> -			['0' ... '9'] = 1,
> -			['A' ... 'Z'] = 1,
> -			['a' ... 'z'] = 1,
> -			['_'] = 1,
> -			['/'] = 1,
> -	};
> +	int index;
> +	static bool initialized;
> +	static char allowed[128];
> +
> +	if (!initialized) {
> +		for (index = '0'; index <= '9'; index++)
> +			allowed[index] = 1;
> +		for (index = 'A'; index <= 'Z'; index++)
> +			allowed[index] = 1;
> +		for (index = 'a'; index <= 'z'; index++)
> +			allowed[index] = 1;
> +		allowed[(int)'_'] = allowed[(int)'/'] = 1;
> +		initialized = true;
> +	}
> +
>  	while (*name != '\0') {
>  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
>  			return false;

Wonder isn't the same as:
while (*name != 0)
	if (!isascii(name[0] || (!isalnum(name[0])  && name[0] != '_' && name[0] != '/'))
		return false; 

Or MSVC doesn't support these  macros?
 


> --
> 1.8.3.1

Re: [PATCH v3] telemetry: use portable syntax to initialize array

[Tyler Retzlaff]

On Tue, Apr 04, 2023 at 09:51:04AM +0100, Bruce Richardson wrote:
> On Mon, Apr 03, 2023 at 11:59:25AM -0700, Tyler Retzlaff wrote:
> > Use of ranges in designated initialization are a non-standard gcc
> > extension. Use loops to initialize permitted characters on first use.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> > ---
> >  lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index 2bac2de..562b387 100644
> > --- a/lib/telemetry/telemetry_data.c
> > +++ b/lib/telemetry/telemetry_data.c
> > @@ -152,13 +152,21 @@
> >  static bool
> >  valid_name(const char *name)
> >  {
> > -	char allowed[128] = {
> > -			['0' ... '9'] = 1,
> > -			['A' ... 'Z'] = 1,
> > -			['a' ... 'z'] = 1,
> > -			['_'] = 1,
> > -			['/'] = 1,
> > -	};
> > +	int index;
> 
> My preference would be to limit the scope of index to the if block, but ok
> to keep as here.

yes, mine too.  but i forgot we aren't C99 (yet) so i had to move it for
this patch.

> 
> [In fact, when we switch to C11, I'd love to see the coding standards
> relaxed to allow loop variable definition inside the for statement itself]

i'm for it, in fact for any variable i prefer declaration at or near
first use because it let's me const more. but i know that can be an
unpopular opinion so no need to hit 'r' to tell me off, i will follow
any and all documented convention the community has.

> 
> > +	static bool initialized;
> > +	static char allowed[128];
> > +
> > +	if (!initialized) {
> > +		for (index = '0'; index <= '9'; index++)
> > +			allowed[index] = 1;
> > +		for (index = 'A'; index <= 'Z'; index++)
> > +			allowed[index] = 1;
> > +		for (index = 'a'; index <= 'z'; index++)
> > +			allowed[index] = 1;
> > +		allowed[(int)'_'] = allowed[(int)'/'] = 1;
> > +		initialized = true;
> > +	}
> > +
> >  	while (*name != '\0') {
> >  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> >  			return false;
> > -- 
> > 1.8.3.1
> > 

Re: [PATCH v3] telemetry: use portable syntax to initialize array

[Tyler Retzlaff]

On Tue, Apr 04, 2023 at 09:01:50AM +0000, Konstantin Ananyev wrote:
> 
> 
> > -----Original Message-----
> > From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > Sent: Monday, April 3, 2023 7:59 PM
> > To: dev@dpdk.org
> > Cc: ciara.power@intel.com; bruce.richardson@intel.com; david.marchand@redhat.com; thomas@monjalon.net; Tyler Retzlaff
> > <roretzla@linux.microsoft.com>
> > Subject: [PATCH v3] telemetry: use portable syntax to initialize array
> > 
> > Use of ranges in designated initialization are a non-standard gcc
> > extension. Use loops to initialize permitted characters on first use.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index 2bac2de..562b387 100644
> > --- a/lib/telemetry/telemetry_data.c
> > +++ b/lib/telemetry/telemetry_data.c
> > @@ -152,13 +152,21 @@
> >  static bool
> >  valid_name(const char *name)
> >  {
> > -	char allowed[128] = {
> > -			['0' ... '9'] = 1,
> > -			['A' ... 'Z'] = 1,
> > -			['a' ... 'z'] = 1,
> > -			['_'] = 1,
> > -			['/'] = 1,
> > -	};
> > +	int index;
> > +	static bool initialized;
> > +	static char allowed[128];
> > +
> > +	if (!initialized) {
> > +		for (index = '0'; index <= '9'; index++)
> > +			allowed[index] = 1;
> > +		for (index = 'A'; index <= 'Z'; index++)
> > +			allowed[index] = 1;
> > +		for (index = 'a'; index <= 'z'; index++)
> > +			allowed[index] = 1;
> > +		allowed[(int)'_'] = allowed[(int)'/'] = 1;
> > +		initialized = true;
> > +	}
> > +
> >  	while (*name != '\0') {
> >  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> >  			return false;
> 
> Wonder isn't the same as:
> while (*name != 0)
> 	if (!isascii(name[0] || (!isalnum(name[0])  && name[0] != '_' && name[0] != '/'))
> 		return false; 
> 
> Or MSVC doesn't support these  macros?

it's standard C msvc supports it. Bruce acceptable to take Konstantin's
suggestion here?

>  
> 
> 
> > --
> > 1.8.3.1

Re: [PATCH v3] telemetry: use portable syntax to initialize array

[Bruce Richardson]

On Tue, Apr 04, 2023 at 08:54:30AM -0700, Tyler Retzlaff wrote:
> On Tue, Apr 04, 2023 at 09:51:04AM +0100, Bruce Richardson wrote:
> > On Mon, Apr 03, 2023 at 11:59:25AM -0700, Tyler Retzlaff wrote:
> > > Use of ranges in designated initialization are a non-standard gcc
> > > extension. Use loops to initialize permitted characters on first use.
> > > 
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > 
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > > ---
> > >  lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
> > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > > index 2bac2de..562b387 100644
> > > --- a/lib/telemetry/telemetry_data.c
> > > +++ b/lib/telemetry/telemetry_data.c
> > > @@ -152,13 +152,21 @@
> > >  static bool
> > >  valid_name(const char *name)
> > >  {
> > > -	char allowed[128] = {
> > > -			['0' ... '9'] = 1,
> > > -			['A' ... 'Z'] = 1,
> > > -			['a' ... 'z'] = 1,
> > > -			['_'] = 1,
> > > -			['/'] = 1,
> > > -	};
> > > +	int index;
> > 
> > My preference would be to limit the scope of index to the if block, but ok
> > to keep as here.
> 
> yes, mine too.  but i forgot we aren't C99 (yet) so i had to move it for
> this patch.
>

We do allow variable declarations within blocks, so no need to move it up
here. However, I believe our coding standards currently require them at the
*top* of each block, not in the middle - but also not just at the top of
each function.  See [1] for the details. [For some reason this is in the
sub-section under "indentation" in the guide!]

[1] https://doc.dpdk.org/guides-22.11/contributing/coding_style.html#local-variables
 
> > 
> > [In fact, when we switch to C11, I'd love to see the coding standards
> > relaxed to allow loop variable definition inside the for statement itself]
> 
> i'm for it, in fact for any variable i prefer declaration at or near
> first use because it let's me const more. but i know that can be an
> unpopular opinion so no need to hit 'r' to tell me off, i will follow
> any and all documented convention the community has.
> 
For const, our coding standards already call this out as an exception,
where it's allowed to define a variable in the middle of a block.
Generally though, I too prefer vars to be defined at site of first use,
rather than all together at the top of the block. Makes it easier when
commenting out code for debugging, not to get unused var warnings.

/Bruce

Re: [PATCH v3] telemetry: use portable syntax to initialize array

[Bruce Richardson]

On Tue, Apr 04, 2023 at 08:59:06AM -0700, Tyler Retzlaff wrote:
> On Tue, Apr 04, 2023 at 09:01:50AM +0000, Konstantin Ananyev wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > Sent: Monday, April 3, 2023 7:59 PM
> > > To: dev@dpdk.org
> > > Cc: ciara.power@intel.com; bruce.richardson@intel.com; david.marchand@redhat.com; thomas@monjalon.net; Tyler Retzlaff
> > > <roretzla@linux.microsoft.com>
> > > Subject: [PATCH v3] telemetry: use portable syntax to initialize array
> > > 
> > > Use of ranges in designated initialization are a non-standard gcc
> > > extension. Use loops to initialize permitted characters on first use.
> > > 
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > >  lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
> > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > > index 2bac2de..562b387 100644
> > > --- a/lib/telemetry/telemetry_data.c
> > > +++ b/lib/telemetry/telemetry_data.c
> > > @@ -152,13 +152,21 @@
> > >  static bool
> > >  valid_name(const char *name)
> > >  {
> > > -	char allowed[128] = {
> > > -			['0' ... '9'] = 1,
> > > -			['A' ... 'Z'] = 1,
> > > -			['a' ... 'z'] = 1,
> > > -			['_'] = 1,
> > > -			['/'] = 1,
> > > -	};
> > > +	int index;
> > > +	static bool initialized;
> > > +	static char allowed[128];
> > > +
> > > +	if (!initialized) {
> > > +		for (index = '0'; index <= '9'; index++)
> > > +			allowed[index] = 1;
> > > +		for (index = 'A'; index <= 'Z'; index++)
> > > +			allowed[index] = 1;
> > > +		for (index = 'a'; index <= 'z'; index++)
> > > +			allowed[index] = 1;
> > > +		allowed[(int)'_'] = allowed[(int)'/'] = 1;
> > > +		initialized = true;
> > > +	}
> > > +
> > >  	while (*name != '\0') {
> > >  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> > >  			return false;
> > 
> > Wonder isn't the same as:
> > while (*name != 0)
> > 	if (!isascii(name[0] || (!isalnum(name[0])  && name[0] != '_' && name[0] != '/'))
> > 		return false; 
> > 
> > Or MSVC doesn't support these  macros?
> 
> it's standard C msvc supports it. Bruce acceptable to take Konstantin's
> suggestion here?
> 
Yep, I'm ok with it.

If I may confuse things a little, how about combining the two? Use an array
with individual initializers for the "special characters", but using
isalnum() for the rest. That way we get a clear list of allowed chars at
the top of the array, without having a massive list of individual letters
and numbers. For example: [completely untested and not compiled!].

{
	/* non-alpha-numeric characters allowed in names */
	static const char allowed[128] = { ['_'] = 1, ['/'] = 1 };

	for ( ; *name != '\0'; name++) {
		if (isalnum(*name))
			continue;
		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
			return false;
	}
	return true;
}

I think something like this achieves a good balance between clarity and
brevity.
1.  We don't have a massive array definition
2.  We do have a static constant array 
3.  We don't have the non-alnum character list hidden inside the middle of
    an "if" statement.

/Bruce

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Tyler Retzlaff]

On Tue, Apr 04, 2023 at 09:47:21AM +0100, Bruce Richardson wrote:
> On Mon, Apr 03, 2023 at 01:19:12PM -0700, Stephen Hemminger wrote:
> > On Mon,  3 Apr 2023 09:30:23 -0700
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > 
> > >  __json_snprintf(char *buf, const int len, const char *format, ...)
> > >  {
> > > -	char tmp[len];
> > > +	char *tmp = malloc(len);
> > >  	va_list ap;
> > > -	int ret;
> > > +	int ret = 0;
> > > +
> > > +	if (tmp == NULL)
> > > +		return ret;
> > >  
> > >  	va_start(ap, format);
> > >  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
> > >  	va_end(ap);
> > >  	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
> > >  		strcpy(buf, tmp);
> > > -		return ret;
> > >  	}
> > > -	return 0; /* nothing written or modified */
> > > +
> > > +	free(tmp);
> > > +
> > > +	return ret;
> > >  }
> > 
> > Not sure why it needs a tmp buffer anyway?
> 
> The temporary buffer is to ensure that in the case that the data doesn't
> fit in the buffer, the buffer remains unmodified. The reason for this is
> that when building up the json response we always have a valid json string.

i guessed this but you've now confirmed it. it makes sense in general
that if the callee signals an error to the caller that the caller shall
not observe any side-effects to do so is to take a dependency on what is
more often than not an internal implementation detail.

> 
> For example, suppose we are preparing a response with an array of two
> strings. After the first string has been processed, the output buffer
> contains: '["string1"]'. When json_snprintf is being called to add string2,
> there are a couple of things to note:
> * the text to be inserted will be put not at the end of the string, but
>   before the closing "]".
> * the actual text to be inserted will be ',"string2"]', so ensuring that
>   the final buffer is valid.
> However, the error case is problematic. While we can catch the case where
> the string to be inserted overflows/has been truncated, doing a regular
> snprintf means that our output buffer could contain invalid json, as our
> end-terminator would have been overwritten, e.g. '["string1","string2'
> To guarantee the output from telemetry is always valid json, even in case
> of truncation, we use a temporary buffer to do the write initially, and if
> it doesn't get truncated, we then copy that to the final buffer.
> 
> That's the logic for this temporary buffer. Now, thinking about it
> yesterday evening, there are other ways in which we can do this, which can
> avoid this temporary buffer.
> 1. We can do the initial snprintf to an empty buffer to get the length that
>    way. This will still be slower, as it means that we need to do printf
>    processing twice rather than using memcpy to copy the result. However, it's
>    probably less overhead than malloc and free.
> 2. AFAIK, the normal case for this function being called is with a single
>    terminator at the end of the string. We can take advantage of that, by
>    checking if the '\0' just one character into the string we are printing,
>    and, if so, to store that once character. If we have a snprintf error
>    leading to truncation, it then allows us to restore the original string.
> 
> My suggestion is to use a combination of these methods. In json_snprintf
> check if the input buffer is empty or has only one character in it, and use
> method #2 if so. If that's not the case, then fallback to method #1 and do
> a double snprintf.
> 
> Make sense? Any other suggestions?

your suggestion seems okay to me, aside from that there's always using
some fixed sized buffer but i'm guessing this being json it's difficult
to choose a reasonable constant size for a stack allocated buffer.

> 
> /Bruce

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Bruce Richardson]

On Tue, Apr 04, 2023 at 09:24:44AM -0700, Tyler Retzlaff wrote:
> On Tue, Apr 04, 2023 at 09:47:21AM +0100, Bruce Richardson wrote:
> > On Mon, Apr 03, 2023 at 01:19:12PM -0700, Stephen Hemminger wrote:
> > > On Mon,  3 Apr 2023 09:30:23 -0700
> > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > > 
> > > >  __json_snprintf(char *buf, const int len, const char *format, ...)
> > > >  {
> > > > -	char tmp[len];
> > > > +	char *tmp = malloc(len);
> > > >  	va_list ap;
> > > > -	int ret;
> > > > +	int ret = 0;
> > > > +
> > > > +	if (tmp == NULL)
> > > > +		return ret;
> > > >  
> > > >  	va_start(ap, format);
> > > >  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
> > > >  	va_end(ap);
> > > >  	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
> > > >  		strcpy(buf, tmp);
> > > > -		return ret;
> > > >  	}
> > > > -	return 0; /* nothing written or modified */
> > > > +
> > > > +	free(tmp);
> > > > +
> > > > +	return ret;
> > > >  }
> > > 
> > > Not sure why it needs a tmp buffer anyway?
> > 
> > The temporary buffer is to ensure that in the case that the data doesn't
> > fit in the buffer, the buffer remains unmodified. The reason for this is
> > that when building up the json response we always have a valid json string.
> 
> i guessed this but you've now confirmed it. it makes sense in general
> that if the callee signals an error to the caller that the caller shall
> not observe any side-effects to do so is to take a dependency on what is
> more often than not an internal implementation detail.
> 
> > 
> > For example, suppose we are preparing a response with an array of two
> > strings. After the first string has been processed, the output buffer
> > contains: '["string1"]'. When json_snprintf is being called to add string2,
> > there are a couple of things to note:
> > * the text to be inserted will be put not at the end of the string, but
> >   before the closing "]".
> > * the actual text to be inserted will be ',"string2"]', so ensuring that
> >   the final buffer is valid.
> > However, the error case is problematic. While we can catch the case where
> > the string to be inserted overflows/has been truncated, doing a regular
> > snprintf means that our output buffer could contain invalid json, as our
> > end-terminator would have been overwritten, e.g. '["string1","string2'
> > To guarantee the output from telemetry is always valid json, even in case
> > of truncation, we use a temporary buffer to do the write initially, and if
> > it doesn't get truncated, we then copy that to the final buffer.
> > 
> > That's the logic for this temporary buffer. Now, thinking about it
> > yesterday evening, there are other ways in which we can do this, which can
> > avoid this temporary buffer.
> > 1. We can do the initial snprintf to an empty buffer to get the length that
> >    way. This will still be slower, as it means that we need to do printf
> >    processing twice rather than using memcpy to copy the result. However, it's
> >    probably less overhead than malloc and free.
> > 2. AFAIK, the normal case for this function being called is with a single
> >    terminator at the end of the string. We can take advantage of that, by
> >    checking if the '\0' just one character into the string we are printing,
> >    and, if so, to store that once character. If we have a snprintf error
> >    leading to truncation, it then allows us to restore the original string.
> > 
> > My suggestion is to use a combination of these methods. In json_snprintf
> > check if the input buffer is empty or has only one character in it, and use
> > method #2 if so. If that's not the case, then fallback to method #1 and do
> > a double snprintf.
> > 
> > Make sense? Any other suggestions?
> 
> your suggestion seems okay to me, aside from that there's always using
> some fixed sized buffer but i'm guessing this being json it's difficult
> to choose a reasonable constant size for a stack allocated buffer.
> 
Yes, choosing a reasonable size is very difficult. We could be snprintf-ing
a string containing a json-ized object a couple of KB long.

I think suggestion #2 above should cover most cases, in which case using
your original suggestion of malloc would be ok too for the rare case (if
ever) where we don't just have one terminator on the end.

/Bruce

Re: [PATCH v3] telemetry: use portable syntax to initialize array

[Tyler Retzlaff]

On Tue, Apr 04, 2023 at 05:19:26PM +0100, Bruce Richardson wrote:
> On Tue, Apr 04, 2023 at 08:59:06AM -0700, Tyler Retzlaff wrote:
> > On Tue, Apr 04, 2023 at 09:01:50AM +0000, Konstantin Ananyev wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > Sent: Monday, April 3, 2023 7:59 PM
> > > > To: dev@dpdk.org
> > > > Cc: ciara.power@intel.com; bruce.richardson@intel.com; david.marchand@redhat.com; thomas@monjalon.net; Tyler Retzlaff
> > > > <roretzla@linux.microsoft.com>
> > > > Subject: [PATCH v3] telemetry: use portable syntax to initialize array
> > > > 
> > > > Use of ranges in designated initialization are a non-standard gcc
> > > > extension. Use loops to initialize permitted characters on first use.
> > > > 
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > ---
> > > >  lib/telemetry/telemetry_data.c | 22 +++++++++++++++-------
> > > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > > > index 2bac2de..562b387 100644
> > > > --- a/lib/telemetry/telemetry_data.c
> > > > +++ b/lib/telemetry/telemetry_data.c
> > > > @@ -152,13 +152,21 @@
> > > >  static bool
> > > >  valid_name(const char *name)
> > > >  {
> > > > -	char allowed[128] = {
> > > > -			['0' ... '9'] = 1,
> > > > -			['A' ... 'Z'] = 1,
> > > > -			['a' ... 'z'] = 1,
> > > > -			['_'] = 1,
> > > > -			['/'] = 1,
> > > > -	};
> > > > +	int index;
> > > > +	static bool initialized;
> > > > +	static char allowed[128];
> > > > +
> > > > +	if (!initialized) {
> > > > +		for (index = '0'; index <= '9'; index++)
> > > > +			allowed[index] = 1;
> > > > +		for (index = 'A'; index <= 'Z'; index++)
> > > > +			allowed[index] = 1;
> > > > +		for (index = 'a'; index <= 'z'; index++)
> > > > +			allowed[index] = 1;
> > > > +		allowed[(int)'_'] = allowed[(int)'/'] = 1;
> > > > +		initialized = true;
> > > > +	}
> > > > +
> > > >  	while (*name != '\0') {
> > > >  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> > > >  			return false;
> > > 
> > > Wonder isn't the same as:
> > > while (*name != 0)
> > > 	if (!isascii(name[0] || (!isalnum(name[0])  && name[0] != '_' && name[0] != '/'))
> > > 		return false; 
> > > 
> > > Or MSVC doesn't support these  macros?
> > 
> > it's standard C msvc supports it. Bruce acceptable to take Konstantin's
> > suggestion here?
> > 
> Yep, I'm ok with it.
> 
> If I may confuse things a little, how about combining the two? Use an array
> with individual initializers for the "special characters", but using
> isalnum() for the rest. That way we get a clear list of allowed chars at
> the top of the array, without having a massive list of individual letters
> and numbers. For example: [completely untested and not compiled!].
> 
> {
> 	/* non-alpha-numeric characters allowed in names */
> 	static const char allowed[128] = { ['_'] = 1, ['/'] = 1 };
> 
> 	for ( ; *name != '\0'; name++) {
> 		if (isalnum(*name))
> 			continue;
> 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> 			return false;
> 	}
> 	return true;
> }
> 
> I think something like this achieves a good balance between clarity and
> brevity.
> 1.  We don't have a massive array definition
> 2.  We do have a static constant array 
> 3.  We don't have the non-alnum character list hidden inside the middle of
>     an "if" statement.

at first glance i'm not sure if that initializer list syntax is portable
but i guess i'll find out.

i'm fine with being confused more so long as you give me the answer and
i can just cut and paste it ;)

> 
> /Bruce

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Tyler Retzlaff]

On Tue, Apr 04, 2023 at 05:28:29PM +0100, Bruce Richardson wrote:
> On Tue, Apr 04, 2023 at 09:24:44AM -0700, Tyler Retzlaff wrote:
> > On Tue, Apr 04, 2023 at 09:47:21AM +0100, Bruce Richardson wrote:
> > > On Mon, Apr 03, 2023 at 01:19:12PM -0700, Stephen Hemminger wrote:
> > > > On Mon,  3 Apr 2023 09:30:23 -0700
> > > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > > > 
> > > > >  __json_snprintf(char *buf, const int len, const char *format, ...)
> > > > >  {
> > > > > -	char tmp[len];
> > > > > +	char *tmp = malloc(len);
> > > > >  	va_list ap;
> > > > > -	int ret;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	if (tmp == NULL)
> > > > > +		return ret;
> > > > >  
> > > > >  	va_start(ap, format);
> > > > >  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
> > > > >  	va_end(ap);
> > > > >  	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
> > > > >  		strcpy(buf, tmp);
> > > > > -		return ret;
> > > > >  	}
> > > > > -	return 0; /* nothing written or modified */
> > > > > +
> > > > > +	free(tmp);
> > > > > +
> > > > > +	return ret;
> > > > >  }
> > > > 
> > > > Not sure why it needs a tmp buffer anyway?
> > > 
> > > The temporary buffer is to ensure that in the case that the data doesn't
> > > fit in the buffer, the buffer remains unmodified. The reason for this is
> > > that when building up the json response we always have a valid json string.
> > 
> > i guessed this but you've now confirmed it. it makes sense in general
> > that if the callee signals an error to the caller that the caller shall
> > not observe any side-effects to do so is to take a dependency on what is
> > more often than not an internal implementation detail.
> > 
> > > 
> > > For example, suppose we are preparing a response with an array of two
> > > strings. After the first string has been processed, the output buffer
> > > contains: '["string1"]'. When json_snprintf is being called to add string2,
> > > there are a couple of things to note:
> > > * the text to be inserted will be put not at the end of the string, but
> > >   before the closing "]".
> > > * the actual text to be inserted will be ',"string2"]', so ensuring that
> > >   the final buffer is valid.
> > > However, the error case is problematic. While we can catch the case where
> > > the string to be inserted overflows/has been truncated, doing a regular
> > > snprintf means that our output buffer could contain invalid json, as our
> > > end-terminator would have been overwritten, e.g. '["string1","string2'
> > > To guarantee the output from telemetry is always valid json, even in case
> > > of truncation, we use a temporary buffer to do the write initially, and if
> > > it doesn't get truncated, we then copy that to the final buffer.
> > > 
> > > That's the logic for this temporary buffer. Now, thinking about it
> > > yesterday evening, there are other ways in which we can do this, which can
> > > avoid this temporary buffer.
> > > 1. We can do the initial snprintf to an empty buffer to get the length that
> > >    way. This will still be slower, as it means that we need to do printf
> > >    processing twice rather than using memcpy to copy the result. However, it's
> > >    probably less overhead than malloc and free.
> > > 2. AFAIK, the normal case for this function being called is with a single
> > >    terminator at the end of the string. We can take advantage of that, by
> > >    checking if the '\0' just one character into the string we are printing,
> > >    and, if so, to store that once character. If we have a snprintf error
> > >    leading to truncation, it then allows us to restore the original string.
> > > 
> > > My suggestion is to use a combination of these methods. In json_snprintf
> > > check if the input buffer is empty or has only one character in it, and use
> > > method #2 if so. If that's not the case, then fallback to method #1 and do
> > > a double snprintf.
> > > 
> > > Make sense? Any other suggestions?
> > 
> > your suggestion seems okay to me, aside from that there's always using
> > some fixed sized buffer but i'm guessing this being json it's difficult
> > to choose a reasonable constant size for a stack allocated buffer.
> > 
> Yes, choosing a reasonable size is very difficult. We could be snprintf-ing
> a string containing a json-ized object a couple of KB long.

haven't checked recently, but i wonder what our normal usermode stack
frame size limit is, which is why alloca() would be scary.

> 
> I think suggestion #2 above should cover most cases, in which case using
> your original suggestion of malloc would be ok too for the rare case (if
> ever) where we don't just have one terminator on the end.

maybe a dumb'd down compromise is to have a fixed stack limit and then
if it is exceeded always just go to malloc/free?

> 
> /Bruce

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Bruce Richardson]

On Tue, Apr 04, 2023 at 09:44:46AM -0700, Tyler Retzlaff wrote:
> On Tue, Apr 04, 2023 at 05:28:29PM +0100, Bruce Richardson wrote:
> > On Tue, Apr 04, 2023 at 09:24:44AM -0700, Tyler Retzlaff wrote:
> > > On Tue, Apr 04, 2023 at 09:47:21AM +0100, Bruce Richardson wrote:
> > > > On Mon, Apr 03, 2023 at 01:19:12PM -0700, Stephen Hemminger wrote:
> > > > > On Mon,  3 Apr 2023 09:30:23 -0700
> > > > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > > > > 
> > > > > >  __json_snprintf(char *buf, const int len, const char *format, ...)
> > > > > >  {
> > > > > > -	char tmp[len];
> > > > > > +	char *tmp = malloc(len);
> > > > > >  	va_list ap;
> > > > > > -	int ret;
> > > > > > +	int ret = 0;
> > > > > > +
> > > > > > +	if (tmp == NULL)
> > > > > > +		return ret;
> > > > > >  
> > > > > >  	va_start(ap, format);
> > > > > >  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
> > > > > >  	va_end(ap);
> > > > > >  	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
> > > > > >  		strcpy(buf, tmp);
> > > > > > -		return ret;
> > > > > >  	}
> > > > > > -	return 0; /* nothing written or modified */
> > > > > > +
> > > > > > +	free(tmp);
> > > > > > +
> > > > > > +	return ret;
> > > > > >  }
> > > > > 
> > > > > Not sure why it needs a tmp buffer anyway?
> > > > 
> > > > The temporary buffer is to ensure that in the case that the data doesn't
> > > > fit in the buffer, the buffer remains unmodified. The reason for this is
> > > > that when building up the json response we always have a valid json string.
> > > 
> > > i guessed this but you've now confirmed it. it makes sense in general
> > > that if the callee signals an error to the caller that the caller shall
> > > not observe any side-effects to do so is to take a dependency on what is
> > > more often than not an internal implementation detail.
> > > 
> > > > 
> > > > For example, suppose we are preparing a response with an array of two
> > > > strings. After the first string has been processed, the output buffer
> > > > contains: '["string1"]'. When json_snprintf is being called to add string2,
> > > > there are a couple of things to note:
> > > > * the text to be inserted will be put not at the end of the string, but
> > > >   before the closing "]".
> > > > * the actual text to be inserted will be ',"string2"]', so ensuring that
> > > >   the final buffer is valid.
> > > > However, the error case is problematic. While we can catch the case where
> > > > the string to be inserted overflows/has been truncated, doing a regular
> > > > snprintf means that our output buffer could contain invalid json, as our
> > > > end-terminator would have been overwritten, e.g. '["string1","string2'
> > > > To guarantee the output from telemetry is always valid json, even in case
> > > > of truncation, we use a temporary buffer to do the write initially, and if
> > > > it doesn't get truncated, we then copy that to the final buffer.
> > > > 
> > > > That's the logic for this temporary buffer. Now, thinking about it
> > > > yesterday evening, there are other ways in which we can do this, which can
> > > > avoid this temporary buffer.
> > > > 1. We can do the initial snprintf to an empty buffer to get the length that
> > > >    way. This will still be slower, as it means that we need to do printf
> > > >    processing twice rather than using memcpy to copy the result. However, it's
> > > >    probably less overhead than malloc and free.
> > > > 2. AFAIK, the normal case for this function being called is with a single
> > > >    terminator at the end of the string. We can take advantage of that, by
> > > >    checking if the '\0' just one character into the string we are printing,
> > > >    and, if so, to store that once character. If we have a snprintf error
> > > >    leading to truncation, it then allows us to restore the original string.
> > > > 
> > > > My suggestion is to use a combination of these methods. In json_snprintf
> > > > check if the input buffer is empty or has only one character in it, and use
> > > > method #2 if so. If that's not the case, then fallback to method #1 and do
> > > > a double snprintf.
> > > > 
> > > > Make sense? Any other suggestions?
> > > 
> > > your suggestion seems okay to me, aside from that there's always using
> > > some fixed sized buffer but i'm guessing this being json it's difficult
> > > to choose a reasonable constant size for a stack allocated buffer.
> > > 
> > Yes, choosing a reasonable size is very difficult. We could be snprintf-ing
> > a string containing a json-ized object a couple of KB long.
> 
> haven't checked recently, but i wonder what our normal usermode stack
> frame size limit is, which is why alloca() would be scary.
> 
> > 
> > I think suggestion #2 above should cover most cases, in which case using
> > your original suggestion of malloc would be ok too for the rare case (if
> > ever) where we don't just have one terminator on the end.
> 
> maybe a dumb'd down compromise is to have a fixed stack limit and then
> if it is exceeded always just go to malloc/free?
> 
Perhaps. If you like, I have have a try at implementing my own suggestions
above tomorrow. I'd like if we can get the "single-character-saving" option
working, because that would be the most efficient method of all.

/Bruce

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Tyler Retzlaff]

On Tue, Apr 04, 2023 at 06:25:42PM +0100, Bruce Richardson wrote:
> On Tue, Apr 04, 2023 at 09:44:46AM -0700, Tyler Retzlaff wrote:
> > On Tue, Apr 04, 2023 at 05:28:29PM +0100, Bruce Richardson wrote:
> > > On Tue, Apr 04, 2023 at 09:24:44AM -0700, Tyler Retzlaff wrote:
> > > > On Tue, Apr 04, 2023 at 09:47:21AM +0100, Bruce Richardson wrote:
> > > > > On Mon, Apr 03, 2023 at 01:19:12PM -0700, Stephen Hemminger wrote:
> > > > > > On Mon,  3 Apr 2023 09:30:23 -0700
> > > > > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > > > > > 
> > > > > > >  __json_snprintf(char *buf, const int len, const char *format, ...)
> > > > > > >  {
> > > > > > > -	char tmp[len];
> > > > > > > +	char *tmp = malloc(len);
> > > > > > >  	va_list ap;
> > > > > > > -	int ret;
> > > > > > > +	int ret = 0;
> > > > > > > +
> > > > > > > +	if (tmp == NULL)
> > > > > > > +		return ret;
> > > > > > >  
> > > > > > >  	va_start(ap, format);
> > > > > > >  	ret = vsnprintf(tmp, sizeof(tmp), format, ap);
> > > > > > >  	va_end(ap);
> > > > > > >  	if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) {
> > > > > > >  		strcpy(buf, tmp);
> > > > > > > -		return ret;
> > > > > > >  	}
> > > > > > > -	return 0; /* nothing written or modified */
> > > > > > > +
> > > > > > > +	free(tmp);
> > > > > > > +
> > > > > > > +	return ret;
> > > > > > >  }
> > > > > > 
> > > > > > Not sure why it needs a tmp buffer anyway?
> > > > > 
> > > > > The temporary buffer is to ensure that in the case that the data doesn't
> > > > > fit in the buffer, the buffer remains unmodified. The reason for this is
> > > > > that when building up the json response we always have a valid json string.
> > > > 
> > > > i guessed this but you've now confirmed it. it makes sense in general
> > > > that if the callee signals an error to the caller that the caller shall
> > > > not observe any side-effects to do so is to take a dependency on what is
> > > > more often than not an internal implementation detail.
> > > > 
> > > > > 
> > > > > For example, suppose we are preparing a response with an array of two
> > > > > strings. After the first string has been processed, the output buffer
> > > > > contains: '["string1"]'. When json_snprintf is being called to add string2,
> > > > > there are a couple of things to note:
> > > > > * the text to be inserted will be put not at the end of the string, but
> > > > >   before the closing "]".
> > > > > * the actual text to be inserted will be ',"string2"]', so ensuring that
> > > > >   the final buffer is valid.
> > > > > However, the error case is problematic. While we can catch the case where
> > > > > the string to be inserted overflows/has been truncated, doing a regular
> > > > > snprintf means that our output buffer could contain invalid json, as our
> > > > > end-terminator would have been overwritten, e.g. '["string1","string2'
> > > > > To guarantee the output from telemetry is always valid json, even in case
> > > > > of truncation, we use a temporary buffer to do the write initially, and if
> > > > > it doesn't get truncated, we then copy that to the final buffer.
> > > > > 
> > > > > That's the logic for this temporary buffer. Now, thinking about it
> > > > > yesterday evening, there are other ways in which we can do this, which can
> > > > > avoid this temporary buffer.
> > > > > 1. We can do the initial snprintf to an empty buffer to get the length that
> > > > >    way. This will still be slower, as it means that we need to do printf
> > > > >    processing twice rather than using memcpy to copy the result. However, it's
> > > > >    probably less overhead than malloc and free.
> > > > > 2. AFAIK, the normal case for this function being called is with a single
> > > > >    terminator at the end of the string. We can take advantage of that, by
> > > > >    checking if the '\0' just one character into the string we are printing,
> > > > >    and, if so, to store that once character. If we have a snprintf error
> > > > >    leading to truncation, it then allows us to restore the original string.
> > > > > 
> > > > > My suggestion is to use a combination of these methods. In json_snprintf
> > > > > check if the input buffer is empty or has only one character in it, and use
> > > > > method #2 if so. If that's not the case, then fallback to method #1 and do
> > > > > a double snprintf.
> > > > > 
> > > > > Make sense? Any other suggestions?
> > > > 
> > > > your suggestion seems okay to me, aside from that there's always using
> > > > some fixed sized buffer but i'm guessing this being json it's difficult
> > > > to choose a reasonable constant size for a stack allocated buffer.
> > > > 
> > > Yes, choosing a reasonable size is very difficult. We could be snprintf-ing
> > > a string containing a json-ized object a couple of KB long.
> > 
> > haven't checked recently, but i wonder what our normal usermode stack
> > frame size limit is, which is why alloca() would be scary.
> > 
> > > 
> > > I think suggestion #2 above should cover most cases, in which case using
> > > your original suggestion of malloc would be ok too for the rare case (if
> > > ever) where we don't just have one terminator on the end.
> > 
> > maybe a dumb'd down compromise is to have a fixed stack limit and then
> > if it is exceeded always just go to malloc/free?
> > 
> Perhaps. If you like, I have have a try at implementing my own suggestions
> above tomorrow. I'd like if we can get the "single-character-saving" option
> working, because that would be the most efficient method of all.

that would be great, i'll take the help i can get.

> 
> /Bruce

[PATCH v4] improve code portability

[Tyler Retzlaff]

Improve portability of telemetry code to allow it to be compiled by msvc
unconditionally.

Remove use of designated initialization ranges (gcc extension). Instead
use a combination of a trivially initialized array and standard C isalnum.

v4:
  * Re-implement using isalnum and sparsely populated array.
v3:
  * Move int index declaration to top of function
  * Fix typo 'Z' -> 'z' in last for loop
v2:
  * Initialize valid character array using for loop instead of explicit
    initialization of each element.
  * Drop patch removing VLAs, in a separate series a conditionally compiled
    change will be introduced.

Tyler Retzlaff (1):
  telemetry: remove non-portable array initialization syntax

 lib/telemetry/telemetry_data.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH v4] telemetry: remove non-portable array initialization syntax

[Tyler Retzlaff]

Use of ranges in designated initialization are a non-standard gcc
extension.

Only initialize '_' and '/' elements of the array and filter tests
of characters through name with standard C isalnum before checking
the array.

Suggested-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/telemetry/telemetry_data.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 2bac2de..0dc091a 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2020 Intel Corporation
  */
 
+#include <ctype.h>
 #include <errno.h>
 #include <stdlib.h>
 #include <inttypes.h>
@@ -152,17 +153,14 @@
 static bool
 valid_name(const char *name)
 {
-	char allowed[128] = {
-			['0' ... '9'] = 1,
-			['A' ... 'Z'] = 1,
-			['a' ... 'z'] = 1,
-			['_'] = 1,
-			['/'] = 1,
-	};
-	while (*name != '\0') {
+	/* non-alpha-numeric characters allowed in names */
+	const char allowed[128] = { ['_'] = 1, ['/'] = 1 };
+
+	for (; *name != '\0'; name++) {
+		if (isalnum(*name))
+			continue;
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
 			return false;
-		name++;
 	}
 	return true;
 }
-- 
1.8.3.1

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Stephen Hemminger]

On Tue, 4 Apr 2023 09:47:21 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> My suggestion is to use a combination of these methods. In json_snprintf
> check if the input buffer is empty or has only one character in it, and use
> method #2 if so. If that's not the case, then fallback to method #1 and do
> a double snprintf.
> 
> Make sense? Any other suggestions?

Glibc has asprintf which allocates the buffer for you.

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Stephen Hemminger]

On Tue, 4 Apr 2023 10:34:01 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> > > > I think suggestion #2 above should cover most cases, in which case using
> > > > your original suggestion of malloc would be ok too for the rare case (if
> > > > ever) where we don't just have one terminator on the end.  
> > > 
> > > maybe a dumb'd down compromise is to have a fixed stack limit and then
> > > if it is exceeded always just go to malloc/free?
> > >   
> > Perhaps. If you like, I have have a try at implementing my own suggestions
> > above tomorrow. I'd like if we can get the "single-character-saving" option
> > working, because that would be the most efficient method of all.  
> 
> that would be great, i'll take the help i can get.


For the json print library in iproute2, it streams output using
stdio, which avoids lots of string processing issues. Well, it moves them
to be standard library.

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Bruce Richardson]

On Tue, Apr 04, 2023 at 06:20:04PM -0700, Stephen Hemminger wrote:
> On Tue, 4 Apr 2023 10:34:01 -0700
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > > > > I think suggestion #2 above should cover most cases, in which case using
> > > > > your original suggestion of malloc would be ok too for the rare case (if
> > > > > ever) where we don't just have one terminator on the end.  
> > > > 
> > > > maybe a dumb'd down compromise is to have a fixed stack limit and then
> > > > if it is exceeded always just go to malloc/free?
> > > >   
> > > Perhaps. If you like, I have have a try at implementing my own suggestions
> > > above tomorrow. I'd like if we can get the "single-character-saving" option
> > > working, because that would be the most efficient method of all.  
> > 
> > that would be great, i'll take the help i can get.
> 
> 
> For the json print library in iproute2, it streams output using
> stdio, which avoids lots of string processing issues. Well, it moves them
> to be standard library.

Yes, but it does have a number of downsides:

* streaming means that we have to worry about message boundaries when
  transferring across sockets like in the telemetry case. With telemetry we
  have a packet-based (message based) interface, so we need to buffer all
  output to a string anyway. If we ever add a streaming (socket or fifo)
  interface, then the iproute2 implementation may be a better fit.

* Last I looked at the iproute2 json implementation, it requires the user to
  track how many outstanding closing brackets of what type are needed,
  meaning that you have invalid json at intermediate stages, and makes it
  awkward if you run out of space in a buffer, i.e. no room for terminators.
  The implementation here, on the other hand, ensures that the output is
  always valid json, so for example, when adding a second string to an
  array of strings we go from ["str1"] to ["str1","str2"] directly - or in
  the case of insufficient space we leave the original version unmodified.
  For working with the case where we have a fixed buffer, this is a massive
  benefit.

/Bruce

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Bruce Richardson]

On Tue, Apr 04, 2023 at 06:04:32PM -0700, Stephen Hemminger wrote:
> On Tue, 4 Apr 2023 09:47:21 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > My suggestion is to use a combination of these methods. In json_snprintf
> > check if the input buffer is empty or has only one character in it, and use
> > method #2 if so. If that's not the case, then fallback to method #1 and do
> > a double snprintf.
> > 
> > Make sense? Any other suggestions?
> 
> Glibc has asprintf which allocates the buffer for you.

Good point, I'll use that in any new implementation. Thanks.

Re: [PATCH v4] telemetry: remove non-portable array initialization syntax

[Bruce Richardson]

On Tue, Apr 04, 2023 at 11:09:16AM -0700, Tyler Retzlaff wrote:
> Use of ranges in designated initialization are a non-standard gcc
> extension.
> 
> Only initialize '_' and '/' elements of the array and filter tests
> of characters through name with standard C isalnum before checking
> the array.
> 
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

The array should probably be "static", which was a miss in the original
version too.

> ---
>  lib/telemetry/telemetry_data.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 2bac2de..0dc091a 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -2,6 +2,7 @@
>   * Copyright(c) 2020 Intel Corporation
>   */
>  
> +#include <ctype.h>
>  #include <errno.h>
>  #include <stdlib.h>
>  #include <inttypes.h>
> @@ -152,17 +153,14 @@
>  static bool
>  valid_name(const char *name)
>  {
> -	char allowed[128] = {
> -			['0' ... '9'] = 1,
> -			['A' ... 'Z'] = 1,
> -			['a' ... 'z'] = 1,
> -			['_'] = 1,
> -			['/'] = 1,
> -	};
> -	while (*name != '\0') {
> +	/* non-alpha-numeric characters allowed in names */
> +	const char allowed[128] = { ['_'] = 1, ['/'] = 1 };
> +
> +	for (; *name != '\0'; name++) {
> +		if (isalnum(*name))
> +			continue;
>  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
>  			return false;
> -		name++;
>  	}
>  	return true;
>  }
> -- 
> 1.8.3.1
> 

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 09:54:46AM +0100, Bruce Richardson wrote:
> On Tue, Apr 04, 2023 at 06:04:32PM -0700, Stephen Hemminger wrote:
> > On Tue, 4 Apr 2023 09:47:21 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > 
> > > My suggestion is to use a combination of these methods. In json_snprintf
> > > check if the input buffer is empty or has only one character in it, and use
> > > method #2 if so. If that's not the case, then fallback to method #1 and do
> > > a double snprintf.
> > > 
> > > Make sense? Any other suggestions?
> > 
> > Glibc has asprintf which allocates the buffer for you.
> 
> Good point, I'll use that in any new implementation. Thanks.

i imagine there is an equivalent to asprintf for windows but keep in
mind it is not standard C so you'll have to do something conditional.

Re: [PATCH v4] telemetry: remove non-portable array initialization syntax

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 09:56:24AM +0100, Bruce Richardson wrote:
> On Tue, Apr 04, 2023 at 11:09:16AM -0700, Tyler Retzlaff wrote:
> > Use of ranges in designated initialization are a non-standard gcc
> > extension.
> > 
> > Only initialize '_' and '/' elements of the array and filter tests
> > of characters through name with standard C isalnum before checking
> > the array.
> > 
> > Suggested-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> > Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> The array should probably be "static", which was a miss in the original
> version too.

micro optimization trade off image size vs stack usage yeah. i'd be inclined
to use static so i can update that.

> 
> > ---
> >  lib/telemetry/telemetry_data.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index 2bac2de..0dc091a 100644
> > --- a/lib/telemetry/telemetry_data.c
> > +++ b/lib/telemetry/telemetry_data.c
> > @@ -2,6 +2,7 @@
> >   * Copyright(c) 2020 Intel Corporation
> >   */
> >  
> > +#include <ctype.h>
> >  #include <errno.h>
> >  #include <stdlib.h>
> >  #include <inttypes.h>
> > @@ -152,17 +153,14 @@
> >  static bool
> >  valid_name(const char *name)
> >  {
> > -	char allowed[128] = {
> > -			['0' ... '9'] = 1,
> > -			['A' ... 'Z'] = 1,
> > -			['a' ... 'z'] = 1,
> > -			['_'] = 1,
> > -			['/'] = 1,
> > -	};
> > -	while (*name != '\0') {
> > +	/* non-alpha-numeric characters allowed in names */
> > +	const char allowed[128] = { ['_'] = 1, ['/'] = 1 };
> > +
> > +	for (; *name != '\0'; name++) {
> > +		if (isalnum(*name))
> > +			continue;
> >  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> >  			return false;
> > -		name++;
> >  	}
> >  	return true;
> >  }
> > -- 
> > 1.8.3.1
> > 

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Dmitry Kozlyuk]

2023-04-05 08:25 (UTC-0700), Tyler Retzlaff:
> On Wed, Apr 05, 2023 at 09:54:46AM +0100, Bruce Richardson wrote:
> > On Tue, Apr 04, 2023 at 06:04:32PM -0700, Stephen Hemminger wrote:  
> > > On Tue, 4 Apr 2023 09:47:21 +0100
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > >   
> > > > My suggestion is to use a combination of these methods. In json_snprintf
> > > > check if the input buffer is empty or has only one character in it, and use
> > > > method #2 if so. If that's not the case, then fallback to method #1 and do
> > > > a double snprintf.
> > > > 
> > > > Make sense? Any other suggestions?  
> > > 
> > > Glibc has asprintf which allocates the buffer for you.  
> > 
> > Good point, I'll use that in any new implementation. Thanks.  
> 
> i imagine there is an equivalent to asprintf for windows but keep in
> mind it is not standard C so you'll have to do something conditional.

There's eal_asprintf() shim in EAL,
we could make it internal to reuse in telemetry.

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Stephen Hemminger]

On Wed, 5 Apr 2023 18:30:51 +0300
Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:

> 2023-04-05 08:25 (UTC-0700), Tyler Retzlaff:
> > On Wed, Apr 05, 2023 at 09:54:46AM +0100, Bruce Richardson wrote:  
> > > On Tue, Apr 04, 2023 at 06:04:32PM -0700, Stephen Hemminger wrote:    
> > > > On Tue, 4 Apr 2023 09:47:21 +0100
> > > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > >     
> > > > > My suggestion is to use a combination of these methods. In json_snprintf
> > > > > check if the input buffer is empty or has only one character in it, and use
> > > > > method #2 if so. If that's not the case, then fallback to method #1 and do
> > > > > a double snprintf.
> > > > > 
> > > > > Make sense? Any other suggestions?    
> > > > 
> > > > Glibc has asprintf which allocates the buffer for you.    
> > > 
> > > Good point, I'll use that in any new implementation. Thanks.    
> > 
> > i imagine there is an equivalent to asprintf for windows but keep in
> > mind it is not standard C so you'll have to do something conditional.  
> 
> There's eal_asprintf() shim in EAL,
> we could make it internal to reuse in telemetry.

Windows has tools necessary to build asprint equivalent.

See: https://github.com/eiszapfen2000/asprintf

Re: [PATCH 1/2] telemetry: use malloc instead of variable length array

[Bruce Richardson]

On Wed, Apr 05, 2023 at 08:25:21AM -0700, Tyler Retzlaff wrote:
> On Wed, Apr 05, 2023 at 09:54:46AM +0100, Bruce Richardson wrote:
> > On Tue, Apr 04, 2023 at 06:04:32PM -0700, Stephen Hemminger wrote:
> > > On Tue, 4 Apr 2023 09:47:21 +0100
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > 
> > > > My suggestion is to use a combination of these methods. In json_snprintf
> > > > check if the input buffer is empty or has only one character in it, and use
> > > > method #2 if so. If that's not the case, then fallback to method #1 and do
> > > > a double snprintf.
> > > > 
> > > > Make sense? Any other suggestions?
> > > 
> > > Glibc has asprintf which allocates the buffer for you.
> > 
> > Good point, I'll use that in any new implementation. Thanks.
> 
> i imagine there is an equivalent to asprintf for windows but keep in
> mind it is not standard C so you'll have to do something conditional.

Only noticed this now, after I've just sent out the patches! I'll have to
see about more rework so. :-(

/Bruce

[PATCH v5] improve code portability

[Tyler Retzlaff]

Improve portability of telemetry code to allow it to be compiled by msvc
unconditionally.

Remove use of designated initialization ranges (gcc extension). Instead
use a combination of a trivially initialized array and standard C isalnum.

v5:
  * declare allowed array static
  * fix alpha-numeric -> alphanumeric (ci warning)

v4:
  * Re-implement using isalnum and sparsely populated array.
v3:
  * Move int index declaration to top of function
  * Fix typo 'Z' -> 'z' in last for loop
v2:
  * Initialize valid character array using for loop instead of explicit
    initialization of each element.
  * Drop patch removing VLAs, in a separate series a conditionally compiled
    change will be introduced.

Tyler Retzlaff (1):
  telemetry: remove non-portable array initialization syntax

 lib/telemetry/telemetry_data.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH v5] telemetry: remove non-portable array initialization syntax

[Tyler Retzlaff]

Use of ranges in designated initialization are a non-standard gcc
extension.

Only initialize '_' and '/' elements of the array and filter tests
of characters through name with standard C isalnum before checking
the array.

Suggested-by: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_data.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 2bac2de..0c7187b 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2020 Intel Corporation
  */
 
+#include <ctype.h>
 #include <errno.h>
 #include <stdlib.h>
 #include <inttypes.h>
@@ -152,17 +153,14 @@
 static bool
 valid_name(const char *name)
 {
-	char allowed[128] = {
-			['0' ... '9'] = 1,
-			['A' ... 'Z'] = 1,
-			['a' ... 'z'] = 1,
-			['_'] = 1,
-			['/'] = 1,
-	};
-	while (*name != '\0') {
+	/* non-alphanumeric characters allowed in names */
+	static const char allowed[128] = { ['_'] = 1, ['/'] = 1 };
+
+	for (; *name != '\0'; name++) {
+		if (isalnum(*name))
+			continue;
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
 			return false;
-		name++;
 	}
 	return true;
 }
-- 
1.8.3.1

Re: [PATCH v5] telemetry: remove non-portable array initialization syntax

[Thomas Monjalon]

05/04/2023 20:52, Tyler Retzlaff:
> Use of ranges in designated initialization are a non-standard gcc
> extension.
> 
> Only initialize '_' and '/' elements of the array and filter tests
> of characters through name with standard C isalnum before checking
> the array.
> 
> Suggested-by: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks.