[0/2] improve code portability

  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(-)
  

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
  

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
  

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(-)
  

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(-)
  

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(-)
  

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(-)