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

  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@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/telemetry/rte_telemetry.h  | 47 ++++++++++++++++++++++
 lib/telemetry/telemetry_data.c | 72 ++++++++++++++++++++++++++++++++++
 lib/telemetry/version.map      |  9 +++++
 3 files changed, 128 insertions(+)
  

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@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Chengwen Feng <fengchengwen@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.

> +	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.

Thanks,
/Bruce
  

在 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@huawei.com>
>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>> Acked-by: Chengwen Feng <fengchengwen@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
> .
  

> From: lihuisong (C) [mailto:lihuisong@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@huawei.com>
> >> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >> Acked-by: Chengwen Feng <fengchengwen@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.

> >
> >> +	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.

-Morten
  

On Thu, Dec 15, 2022 at 01:00:40PM +0100, Morten Brørup wrote:
> > From: lihuisong (C) [mailto:lihuisong@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@huawei.com>
> > >> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > >> Acked-by: Chengwen Feng <fengchengwen@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.

/Bruce
  

> From: Bruce Richardson [mailto:bruce.richardson@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@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@huawei.com>
> > > >> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > >> Acked-by: Chengwen Feng <fengchengwen@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.

-Morten
  

在 2022/12/15 20:24, Morten Brørup 写道:
>> From: Bruce Richardson [mailto:bruce.richardson@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@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@huawei.com>
>>>>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>>>>> Acked-by: Chengwen Feng <fengchengwen@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
  

> From: lihuisong (C) [mailto:lihuisong@huawei.com]
> Sent: Thursday, 15 December 2022 13.46
> 
> 在 2022/12/15 20:24, Morten Brørup 写道:
> >> From: Bruce Richardson [mailto:bruce.richardson@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@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@huawei.com>
> >>>>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >>>>>> Acked-by: Chengwen Feng <fengchengwen@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?

I had the same concern, so I looked it up.

snprintf() returns the length that the string would have, regardless if it was truncated or not.

In other words:

If the string is truncated, snprintf() returns a value greater than the buffer length parameter given to it.

It can be checked like this:

if (snprintf(buf, len, "0x%" PRIx64, val) > len)
    return -EINVAL;

In my opinion, checking for negative return values from snprintf() is not necessary.
  

On Thu, Dec 15, 2022 at 01:52:02PM +0100, Morten Brørup wrote:
> > From: lihuisong (C) [mailto:lihuisong@huawei.com]
> > Sent: Thursday, 15 December 2022 13.46
> > 
> > 在 2022/12/15 20:24, Morten Brørup 写道:
> > >> From: Bruce Richardson [mailto:bruce.richardson@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@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@huawei.com>
> > >>>>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > >>>>>> Acked-by: Chengwen Feng <fengchengwen@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?
> 
> I had the same concern, so I looked it up.
> 
> snprintf() returns the length that the string would have, regardless if it was truncated or not.
> 
> In other words:
> 
> If the string is truncated, snprintf() returns a value greater than the buffer length parameter given to it.
> 
> It can be checked like this:
> 
> if (snprintf(buf, len, "0x%" PRIx64, val) > len)
>     return -EINVAL;
> 
> In my opinion, checking for negative return values from snprintf() is not necessary.
>
+1
One nit, because of the \0, we need to check for ">=" len since a return val
equal to length means the last character was truncated to make room for the
\0.

/Bruce
  

在 2022/12/15 21:08, Bruce Richardson 写道:
> On Thu, Dec 15, 2022 at 01:52:02PM +0100, Morten Brørup wrote:
>>> From: lihuisong (C) [mailto:lihuisong@huawei.com]
>>> Sent: Thursday, 15 December 2022 13.46
>>>
>>> 在 2022/12/15 20:24, Morten Brørup 写道:
>>>>> From: Bruce Richardson [mailto:bruce.richardson@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@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@huawei.com>
>>>>>>>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>>>>>>>> Acked-by: Chengwen Feng <fengchengwen@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?
>> I had the same concern, so I looked it up.
>>
>> snprintf() returns the length that the string would have, regardless if it was truncated or not.
>>
>> In other words:
>>
>> If the string is truncated, snprintf() returns a value greater than the buffer length parameter given to it.
>>
>> It can be checked like this:
>>
>> if (snprintf(buf, len, "0x%" PRIx64, val) > len)
>>      return -EINVAL;
>>
>> In my opinion, checking for negative return values from snprintf() is not necessary.
>>
> +1
> One nit, because of the \0, we need to check for ">=" len since a return val
> equal to length means the last character was truncated to make room for the
> \0.
ok, do this in this way.