On Thu, Dec 15, 2022 at 01:52:02PM +0100, Morten Brørup wrote: > > From: lihuisong (C) [mailto:lihuis...@huawei.com] > > Sent: Thursday, 15 December 2022 13.46 > > > > 在 2022/12/15 20:24, Morten Brørup 写道: > > >> 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. > > 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
