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