Re: [PATCH 0/2] improve code portability

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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