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

[lihuisong (C)]

在 2022/12/15 20:24, Morten Brørup 写道:
>> From: Bruce Richardson [mailto:bruce.richardson at intel.com]
>> Sent: Thursday, 15 December 2022 13.16
>>
>> On Thu, Dec 15, 2022 at 01:00:40PM +0100, Morten Brørup wrote:
>>>> From: lihuisong (C) [mailto:lihuisong at huawei.com]
>>>> Sent: Thursday, 15 December 2022 12.28
>>>>
>>>> 在 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,
>>> The "len" parameter should be size_t or unsigned int, not uint16_t.
>>>
>>> And as a personal preference, I would name it "buf_len" instead of
>> "len".
>>>>>> +				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;
>>> LGTM.
>> That would work, but actually I think we should drop this check
>> completely
>> - see comment below.
>>
>>>>>> +	if (display_bitwidth != 0) {
>>>>>> +		sprintf(format, "0x%%0%u" PRIx64, spec_hex_width);
>>>>>> +		sprintf(buf, format, val);
>>> snprintf(format, sizeof(format), "0x%%0%u" PRIx64, spec_hex_width);
>>> snprintf(buf, len, format, val);
>>>
>>>>>> +	} else {
>>>>>> +		sprintf(buf, "0x%" PRIx64, val);
>>> snprintf(buf, len, "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?
>>> You already check the buffer size before printf() into it, so I think
>> it suffices. However, to keep automated code checkers happy, you could
>> easily use snprintf() instead of printf(). (Sorry about not doing it in
>> my example.)
>>> I somewhat disagree with Bruce's suggestion to check the return value
>> from snprintf() after checking that the buffer is large enough. It
>> would be effectively dead code, which might cause some confusion. On
>> the other hand, it might keep some automated code checkers happy. In
>> this specific case here, I don't have a strong preference.
>> Sure, you don't need 2 checks - we can either check the length at the
>> start, or else check the length by looking for the return value from
>> snprintf, but we don't need to do both.
>>
>> Given the slight complexity in determining the correct length of the
>> printf
>> for the initial size check, I think I would go with the approach of
>> *not*
>> checking the buffer initially and just check the snprintf return value.
>> That would remove any possibility of us doing an incorrect length
>> check, as
>> was the case originally with this patch.
> That sounds reasonable to me. Please do that.
I think above check is necessary. Because snprintf returns the total 
length of string
formated instead of negative when buffer size isn't sufficient. what do 
you think?

/Huisong