在 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: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.
ok, do this in this way.
