在 2022/12/12 20:03, Morten Brørup 写道:
From: Bruce Richardson [mailto:bruce.richard...@intel.com]
Sent: Monday, 12 December 2022 12.21
On Mon, Dec 12, 2022 at 12:02:32PM +0100, Morten Brørup wrote:
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.
I think just using the u64 functions is probably simplest and best
right
now. If we add support for something like snmp then yes, it would make
sense to explicitly add it, but it seems like a lot of extra code for
little or no benefit until we support something like that.
<rant>
If we wanted to fix this generally, we should rely on type promotion, so the
existing _int function should be updated to take an int64_t value, and the _u64
function should be renamed to _uint (and still take an uint64_t value).
However, that would break the ABI, and would need to go through some process
for that. So let's not change this now.
</rant>
I tend to agree with Bruce on this: Let's get rid of the new u32 functions, and
rely on the u64 functions for that instead.
All right. Let's drop the new u32 functions.
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.
+1 to having only a single hex function, and I think type promotion
should
work fine.
However, I still think it might be worthwhile allowing the user to pass
in
a min output length parameter too. I find for many hex strings having
the
leading zeros to explicitly show the length can be useful. The value
"0"
could cover the current behaviour of no-padding, otherwise the
parameter
should indicate the number of bits to be displayed. (If we want to lock
it
down we can use an enum parameter rather than int to limit it to 0, 8,
16,
32 or 64 bit displayed values).
An extra parameter for minimum output length (in number of input bits) would be
very nice, and makes avoids a set of functions for each bit width.
(I don't think it should be lock it down to some special bit lengths; there is
no need to prevent 24 bit or other exotic bit widths.)
Something like this:
char str[64]; // Big enough length.
if (bits != 0) {
char format[16]; // Big enough length.
sprintf(format, "0x0%u%" PRIx64, (bits + 3) / 4);
sprintf(str, format, value);
} else {
sprintf(str, "0x%" PRIx64, value);
}
Fix it in v4.
All that said, I'm not massively concerned if we want to just keep the
current approach of always just printing without any leading zeros.
It's a
nice-to-have only for me.
/Bruce
.
