在 2022/12/14 15:28, Morten Brørup 写道:
From: lihuisong (C) [mailto:lihuis...@huawei.com]
Sent: Wednesday, 14 December 2022 03.44
在 2022/12/14 1:09, Bruce Richardson 写道:
On Tue, Dec 13, 2022 at 06:15:10PM +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
(can be
one of uint8_t, uint16_t, uint32_t and uint64_t type) value as
hexadecimal
encoded string to array or dictionary. If the 'val_bits' is zero,
the value
is stored as hexadecimal encoded string without padding zero.
Signed-off-by: Huisong Li <lihuis...@huawei.com>
Acked-by: Morten Brørup <m...@smartsharesystems.com>
Acked-by: Chengwen Feng <fengcheng...@huawei.com>
Thanks for the patch. Agree with the principle of it, but some
comments
inline.
/Bruce
---
lib/telemetry/rte_telemetry.h | 50 +++++++++++++++++++++++++++++
lib/telemetry/telemetry_data.c | 58
++++++++++++++++++++++++++++++++++
lib/telemetry/version.map | 9 ++++++
3 files changed, 117 insertions(+)
diff --git a/lib/telemetry/rte_telemetry.h
b/lib/telemetry/rte_telemetry.h
index 40e9a3bf9d..88b34097b0 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -10,6 +10,7 @@ extern "C" {
#endif
#include <stdint.h>
+#include <rte_compat.h>
/** Maximum length for string used in object. */
#define RTE_TEL_MAX_STRING_LEN 128
@@ -20,6 +21,11 @@ extern "C" {
/** Maximum number of array entries. */
#define RTE_TEL_MAX_ARRAY_ENTRIES 512
+#define RTE_TEL_U8_BITS 8
+#define RTE_TEL_U16_BITS 16
+#define RTE_TEL_U32_BITS 32
+#define RTE_TEL_U64_BITS 64
+
Not sure these are really necessary, but fairly harmless I suppose.
This is convenient for the user to use.
/**
* @file
*
@@ -153,6 +159,27 @@ int
rte_tel_data_add_array_container(struct rte_tel_data *d,
struct rte_tel_data *val, int keep);
+/**
+ * Convert a unsigned integer to hexadecimal encoded strings and
add this string
+ * to an array.
+ * The array must have been started by rte_tel_data_start_array()
with
+ * RTE_TEL_STRING_VAL as the type parameter.
+ *
+ * @param d
+ * The data structure passed to the callback
+ * @param val
+ * The number to be returned in the array as a hexadecimal
encoded strings.
+ * The type of ''val' can be one of uint8_t, uint16_t, uint32_t
and uint64_t.
Not sure this last line needs to be stated.
+ * @param val_bits
+ * The total bits of the input 'val'. If val_bits is zero, the
value is stored
+ * in the array as hexadecimal encoded string without padding
zero.
+ * @return
+ * 0 on success, negative errno on error
+ */
+__rte_experimental
+int rte_tel_data_add_array_uint_hex(struct rte_tel_data *d,
uint64_t val,
+ uint16_t val_bits);
+
Just watch for whitespace consistency and coding standards. The "int"
should be on a line by itself, so the function name always starts in
column 0 of a line.
Sorry, I refer to a wrong example.
I would also suggest renaming "val_bits" - maybe "display_bitwidth"
would
be clearer, though also rather long.
The 'val_bits' means the total bits of input 'val', It also reflects
the
type of data
to be stored in hexadecimal. After all, we use 'u64' to cover all
unisgned integer types.
And this function is introduced for adding hexadecimal format value,
not
binary format.
The value can not be stored exactly according to the input
"display_bitwidth".
If we limit to only 8/16/32/64 integer types, the 'val_bits' is better,
I think.
NAK to limiting the bitwidth to 8/16/32/64 bits!
The function should be able to dump bitfields, such as the TCP flags, where the
bitwidth is 6.
/**
* Add a string value to a dictionary.
* The dict must have been started by rte_tel_data_start_dict().
@@ -231,6 +258,29 @@ int
rte_tel_data_add_dict_container(struct rte_tel_data *d, const char
*name,
struct rte_tel_data *val, int keep);
+/**
+ * Convert a unsigned integer to hexadecimal encoded strings and
add this string
+ * to an dictionary.
+ * The dict must have been started by rte_tel_data_start_dict().
+ *
+ * @param d
+ * The data structure passed to the callback
+ * @param name
+ * The name of the value is to be stored in the dict
+ * Must contain only alphanumeric characters or the symbols: '_'
or '/'
+ * @param val
+ * The number to be stored in the dict as a hexadecimal encoded
strings.
+ * The type of ''val' can be one of uint8_t, uint16_t, uint32_t
and uint64_t.
+ * @param val_bits
+ * The total bits of the input 'val'. If val_bits is zero, the
value is stored
+ * in the array as hexadecimal encoded string without padding
zero.
+ * @return
+ * 0 on success, negative errno on error
+ */
+__rte_experimental
+int rte_tel_data_add_dict_uint_hex(struct rte_tel_data *d, const
char *name,
+ uint64_t val, uint16_t val_bits);
+
same comments as above.
/**
* This telemetry callback is used when registering a telemetry
command.
* It handles getting and formatting information to be returned to
telemetry
diff --git a/lib/telemetry/telemetry_data.c
b/lib/telemetry/telemetry_data.c
index 080d99aec9..fb2f711d99 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -4,6 +4,7 @@
#include <errno.h>
#include <stdlib.h>
+#include <inttypes.h>
#undef RTE_USE_LIBBSD
#include <stdbool.h>
@@ -12,6 +13,9 @@
#include "telemetry_data.h"
+#define RTE_TEL_UINT_HEX_STRING_BUFFER_LEN 64
+#define RTE_TEL_UINT_HEX_FORMAT_LEN 16
+
int
rte_tel_data_start_array(struct rte_tel_data *d, enum
rte_tel_value_type type)
{
@@ -113,6 +117,33 @@ rte_tel_data_add_array_container(struct
rte_tel_data *d,
return 0;
}
+int
+rte_tel_data_add_array_uint_hex(struct rte_tel_data *d, uint64_t
val,
+ uint16_t val_bits)
+{
+ char hex_str[RTE_TEL_UINT_HEX_STRING_BUFFER_LEN];
+
+ switch (val_bits) {
+ case RTE_TEL_U8_BITS:
+ sprintf(hex_str, "0x%02"PRIx64"", val);
+ break;
+ case RTE_TEL_U16_BITS:
+ sprintf(hex_str, "0x%04"PRIx64"", val);
+ break;
+ case RTE_TEL_U32_BITS:
+ sprintf(hex_str, "0x%08"PRIx64"", val);
+ break;
+ case RTE_TEL_U64_BITS:
+ sprintf(hex_str, "0x%016"PRIx64"", val);
+ break;
+ default:
+ sprintf(hex_str, "0x%"PRIx64"", val);
+ break;
+ }
+
+ return rte_tel_data_add_array_string(d, hex_str);
+}
+
This assume we only want those power-of-2 sizes. Is there a reason
why we
can't use the code suggested by Morten in the discussion on v3?
Having the
extra flexibility might be nice if we can get it.
The compiler doesn't like it. There is a warning:
'warning: format not a string literal, argument types not checked
[-Wformat-nonliteral]'
You can surround the affected functions by some #pragma to temporarily disable
that warning.
Good idea! I will fix it in V5.
I assume you noticed the bugs in my code:
char str[64]; // FIXME: Use correct size.
if (bits != 0) {
char format[16]; // FIXME: Use correct size.
sprintf(format, "0x%%0%u" PRIx64, (bits + 3) / 4); // bug fixed here
sprintf(str, format, value);
} else {
sprintf(str, "0x%" PRIx64, value);
}
If we do need to limit to only 8/16/32/64, then I recommend using an
enum
in the header rather than #defines for those values. That makes it
clearer
that only a very limited range is supported.
Also, code above treats all values other than 8/16/32/64 as if they
were 0.
If 20, for example, is passed, we probably want to return error
rather than
treating it as zero.
I have to only consider 8/16/32/64 integer types because of above
warning.
In addition, the normal unsigned integer data is one of them. If user
forces
'0xf1f23' value to 10 bitwidth to display, it will be truncated as
0xf23.
The printf width field specifies the MINIMUM number of characters to output.
Truncation would be a bug in the C library.
It seems pointless and unfriendly.
So overall, this function is limited to all uint types, and is
currently
fully adequate.
Do we need to check other bitwidth? If the 'val_bits' isn't 8/16/32/64,
it is processed as
no-padding zero.
