> From: Bruce Richardson [mailto:bruce.richard...@intel.com] > Sent: Monday, 12 December 2022 11.32 > > On Mon, Dec 12, 2022 at 02:42:55PM +0800, Huisong Li wrote: > > Some lib telemetry interfaces add the 'u32' and 'u64' data by the > > rte_tel_data_add_dict/array_int API. This may cause data conversion > error > > or data truncation. > > > > The 'u32' data can not be assigned to signed 32-bit integer. However, > > assigning to u64 is very wasteful, after all, the buffer capacity of > each > > transfer is limited. So it is necessary for 'u32' data to add usigned > > 32-bit integer type and a series of 'u32' operation APIs. > > > > This patchset uses the new 'u32' API to resolve the problem of data > > conversion error, and use the 'u64' API to add 'u64' data. > > > > In addition, this patchset introduces two APIs to store u32 and u64 > > values as hexadecimal encoded strings in telemetry library. > > > > --- -v3: fix a misspelling mistake in commit log. -v2: - fix ABI > break > > warning. - introduce two APIs to store u32 and u64 values as > hexadecimal > > encoded strings. > > > I'm not convinced about adding the u32 value generically to the > telemetry > lib - except in the case of having explicit function calls for u32 vs > u64 > hex strings. Having a u32 type doesn't gain us any space internally > over a > u64 value, since all values are in a union type. Also, for output as > json, > the numeric values are all output as decimal values, meaning that the > value > 1 appears as the same size in the output string whether it is a u32 or > u64 > type. Now, it may save space in a future binary output format, but even > then it still may not do so.
I agree that a u32 doesn't gain any space internally. However, many SNMP counters are unsigned 32 bit, and expected to wrap around as such. So I suppose the u32 type might be useful for SNMP, if obtained through the telemetry library. Alternatively, we could somehow reuse the u64 type and require the application to pass (value & UINT32_MAX) to the u64 functions. To make this easy to use, we should add some wrappers to do it for the application. And eventually we would probably end up with something very similar to this patch. > > Therefore, I'd tend to keep the existing u64 type as-is, and instead > only > add the functions for outputting hex values. Those hex output functions > could take an additional parameter indicating the desired hex output > length, as there could well be cases where we want just 16-bit hex > value > too. The way I read the patch series, the hex output length is not fixed, but an u64 value of 5 will be output as 0x5, not 0x0000000000000005. So the only benefit of having both u32_hex and u64_hex functions is to avoid type promotion from uint32_t to uint64_t on input for 32 bit values. Instead of passing a 3rd parameter or adding u16_hex functions, we could consider just having one set of hex functions using uint64_t for the value, and rely on type promotion for 32 and 16 bit values.
