On Fri, Mar 10, 2023 at 11:08:32AM -0800, Stephen Hemminger wrote: > On Fri, 10 Mar 2023 18:18:36 +0000 > Bruce Richardson <[email protected]> wrote: > > > On Alpine linux, the telemetry_data_autotest was failing for the > > test where we had dictionaries embedded in other dictionaries up > > to three levels deep. Indications are that this issue is due to > > excess data being stored on the stack, so replace stack-allocated > > buffer data with dynamically allocated data in the case where we > > are doing recursive processing of telemetry data structures into > > json. > > > > Bugzilla ID: 1177 > > Fixes: c933bb5177ca ("telemetry: support array values in data object") > > Fixes: d2671e642a8e ("telemetry: support dict of dicts") > > Cc: [email protected] > > Looking at the telemetry code: > > - why so many temporary buffers, could this be streamed or redesigned > so that an allocated buffer is returned. >
This is largely what the fix patch does. The temporary buffers are used to ensure we never end up with a partially written buffer from any truncated snprintf, since that could cause us to emit invalid json. I think as a scheme it works well enough for what it was designed for, but this particular test case has more levels of recursion than was expected, so the limit of the design are showing. The downside of using memory allocation using malloc as here, is just more failure cases. > - why is rte_tel_json_XXX all inline? These should just be internal > functions and not in a .h file. > Sure. Since these are all just internal functions used by the library, I'm not sure it really matters. > FYI - if this library reused existing json writer it would have > been much simpler. > https://github.com/shemminger/iproute2/blob/main/lib/json_writer.c Yep. Looked at that previously, but it's at a lower level than what we have. IMHO, the complicated bit of producing json output is not the formatting characters for each data-type but ensuring correct termination of the json string even in case of errors, so each function in our telemetry json code always includes correct terminators at all points, i.e. we have no separate functions to be called for terminating objects, arrays etc. - they are always included in the output of the items. This is why so many temporary buffers are used - the input string to a function is well-formed json, and we only actually append to that buffer by copying from the temporary buffer once we are sure that the output will also be similarly well-formed. That said, if you want to replace the current json implementation with a better one, I'm not opposed to it. [though I'd definitely rather no external dependencies which would mean we could no longer rely on it always being available in default builds] /Bruce
