[PATCH V6 6/8] telemetry: support adding integer value as hexadecimal

[lihuisong (C)]

在 2022/12/15 18:46, Bruce Richardson 写道:
> On Thu, Dec 15, 2022 at 06:31:44PM +0800, Huisong Li wrote:
>> Sometimes displaying a unsigned integer value as hexadecimal encoded style
>> is more expected for human consumption, such as, offload capability and
>> device flag. This patch introduces two APIs to add unsigned integer value
>> as hexadecimal encoded string to array or dictionary. And user can choose
>> whether the stored value is padding to the specified width.
>>
>> Signed-off-by: Huisong Li <lihuisong at huawei.com>
>> Acked-by: Morten Brørup <mb at smartsharesystems.com>
>> Acked-by: Chengwen Feng <fengchengwen at huawei.com>
>> ---
>>   lib/telemetry/rte_telemetry.h  | 47 ++++++++++++++++++++++
>>   lib/telemetry/telemetry_data.c | 72 ++++++++++++++++++++++++++++++++++
>>   lib/telemetry/version.map      |  9 +++++
>>   3 files changed, 128 insertions(+)
>>
> <snip>
>> +/* To suppress compiler warning about format string. */
>> +#if defined(RTE_TOOLCHAIN_GCC)
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
>> +#elif defined(RTE_TOOLCHAIN_CLANG)
>> +#pragma clang diagnostic push
>> +#pragma clang diagnostic ignored "-Wformat-nonliteral"
>> +#endif
>> +
>> +static int
>> +rte_tel_uint_to_hex_encoded_str(char *buf, uint16_t len, uint64_t val,
>> +				uint8_t display_bitwidth)
>> +{
>> +#define RTE_TEL_UINT_HEX_FORMAT_LEN 16
>> +
>> +	uint8_t spec_hex_width = (display_bitwidth + 3) / 4;
>> +	char format[RTE_TEL_UINT_HEX_FORMAT_LEN];
>> +
>> +	/* Buffer needs to have room to contain the prefix '0x' and '\0'. */
>> +	if (len < spec_hex_width + 3)
>> +		return -EINVAL;
>> +
> This check is not sufficient, as display_bitwidth could be 0 for example,
> and the actual printed number have up to 16 characters.
Yes, you are right. What do you think of the following check?

max_hex_width = display_bitwidth == 0 ? (sizeof(uint64_t) * 2) : 
spec_hex_width;
if (len < max_hex_width + 3)
     return -EINVAL;
>
>> +	if (display_bitwidth != 0) {
>> +		sprintf(format, "0x%%0%u" PRIx64, spec_hex_width);
>> +		sprintf(buf, format, val);
>> +	} else {
>> +		sprintf(buf, "0x%" PRIx64, val);
>> +	}
>> +
> snprintf should always be used when printing to the buffer so as to avoid
> overflow. The return value after snprintf should always be checked too.
If check for buffer is sufficient, can the return value of snprintf not 
be checked?
There are also many places in telemetry lib that are not checked for 
this return value.
Do you have any principles for this?
>
> Thanks,
> /Bruce
> .