> From: Bruce Richardson [mailto:bruce.richard...@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: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. > > 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
