On Sat, Jun 7, 2025 at 11:15 PM Aaron Conole <acon...@redhat.com> wrote:
>
> Changliang Wu <changliang...@smartx.com> writes:
>
> > snprintf will automatically write \0 at the end of the string,
> > and the last one byte will be out of bound.
> >
> > create a new func format_hex_arg with delimiter in utils,
> > instead of chassisid_to string and format_hex_arg.
> >
> > Found in sanitize test.
> >
> > Signed-off-by: Changliang Wu <changliang...@smartx.com>
> > ---
> >
> > v2: create new function for hex string with delimiter
> >
> > ---
> > lib/ofp-actions.c | 15 ++-------------
> > lib/ofp-packet.c | 13 +------------
> > lib/ovs-lldp.c | 35 ++++++++++-------------------------
> > lib/util.c | 13 +++++++++++++
> > lib/util.h | 4 ++++
> > tests/library.at | 4 ++++
> > tests/test-util.c | 20 ++++++++++++++++++++
> > 7 files changed, 54 insertions(+), 50 deletions(-)
> >
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index 102abbc23..d3c2195ac 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -1023,17 +1023,6 @@ parse_CONTROLLER(char *arg, const struct
> > ofpact_parse_params *pp)
> > return NULL;
> > }
> >
> > -static void
> > -format_hex_arg(struct ds *s, const uint8_t *data, size_t len)
> > -{
> > - for (size_t i = 0; i < len; i++) {
> > - if (i) {
> > - ds_put_char(s, '.');
> > - }
> > - ds_put_format(s, "%02"PRIx8, data[i]);
> > - }
> > -}
> > -
> > static void
> > format_CONTROLLER(const struct ofpact_controller *a,
> > const struct ofpact_format_params *fp)
> > @@ -1063,7 +1052,7 @@ format_CONTROLLER(const struct ofpact_controller *a,
> > }
> > if (a->userdata_len) {
> > ds_put_format(fp->s, "%suserdata=%s", colors.param,
> > colors.end);
> > - format_hex_arg(fp->s, a->userdata, a->userdata_len);
> > + format_hex_arg(fp->s, a->userdata, a->userdata_len, ".");
> > ds_put_char(fp->s, ',');
> > }
> > if (a->pause) {
> > @@ -5960,7 +5949,7 @@ format_NOTE(const struct ofpact_note *a,
> > const struct ofpact_format_params *fp)
> > {
> > ds_put_format(fp->s, "%snote:%s", colors.param, colors.end);
> > - format_hex_arg(fp->s, a->data, a->length);
> > + format_hex_arg(fp->s, a->data, a->length, ".");
> > }
> >
> > static enum ofperr
> > diff --git a/lib/ofp-packet.c b/lib/ofp-packet.c
> > index 9485ddfc9..e1875c84b 100644
> > --- a/lib/ofp-packet.c
> > +++ b/lib/ofp-packet.c
> > @@ -916,17 +916,6 @@ ofputil_decode_packet_in_private(const struct
> > ofp_header *oh, bool loose,
> > return error;
> > }
> >
> > -static void
> > -format_hex_arg(struct ds *s, const uint8_t *data, size_t len)
> > -{
> > - for (size_t i = 0; i < len; i++) {
> > - if (i) {
> > - ds_put_char(s, '.');
> > - }
> > - ds_put_format(s, "%02"PRIx8, data[i]);
> > - }
> > -}
> > -
> > void
> > ofputil_packet_in_private_format(struct ds *s,
> > const struct ofputil_packet_in_private
> > *pin,
> > @@ -973,7 +962,7 @@ ofputil_packet_in_private_format(struct ds *s,
> >
> > if (public->userdata_len) {
> > ds_put_cstr(s, " userdata=");
> > - format_hex_arg(s, pin->base.userdata, pin->base.userdata_len);
> > + format_hex_arg(s, pin->base.userdata, pin->base.userdata_len, ".");
> > ds_put_char(s, '\n');
> > }
> >
> > diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
> > index 2d13e971e..8c9d7890a 100644
> > --- a/lib/ovs-lldp.c
> > +++ b/lib/ovs-lldp.c
> > @@ -101,21 +101,6 @@ static struct hmap *const all_mappings
> > OVS_GUARDED_BY(mutex) = &all_mappings__;
> >
> > static struct lldp_aa_element_system_id system_id_null;
> >
> > -/* Convert an LLDP chassis ID to a string.
> > - */
> > -static void
> > -chassisid_to_string(uint8_t *array, size_t len, char **str)
> > -{
> > - unsigned int i;
> > -
> > - *str = xmalloc(len * 3);
> > -
> > - for (i = 0; i < len; i++) {
> > - snprintf(&(*str)[i * 3], 4, "%02x:", array[i]);
> > - }
> > - (*str)[(i * 3) - 1] = '\0';
> > -}
> > -
> > /* Find an Auto Attach mapping keyed by I-SID.
> > */
> > static struct aa_mapping_internal *
> > @@ -204,30 +189,30 @@ aa_print_element_status_port(struct ds *ds, struct
> > lldpd_hardware *hw)
> > sizeof port->p_element.system_id)) {
> > const char *none_str = "<None>";
> > const char *descr = NULL;
> > - char *id = NULL;
> > - char *system;
> > + struct ds id = DS_EMPTY_INITIALIZER;
> > + struct ds system = DS_EMPTY_INITIALIZER;
> >
> > if (port->p_chassis) {
> > if (port->p_chassis->c_id_len > 0) {
> > - chassisid_to_string(port->p_chassis->c_id,
> > - port->p_chassis->c_id_len, &id);
> > + format_hex_arg(&id, port->p_chassis->c_id,
> > + port->p_chassis->c_id_len, ":");
> > }
> >
> > descr = port->p_chassis->c_descr;
> > }
> >
> > - chassisid_to_string((uint8_t *) &port->p_element.system_id,
> > - sizeof port->p_element.system_id, &system);
> > + format_hex_arg(&system, (uint8_t *) &port->p_element.system_id,
> > + sizeof port->p_element.system_id, ":");
> >
> > ds_put_format(ds, " Auto Attach Primary Server Id: %s\n",
> > - id ? id : none_str);
> > + id.length ? ds_cstr_ro(&id) : none_str);
> > ds_put_format(ds, " Auto Attach Primary Server Descr: %s\n",
> > descr ? descr : none_str);
> > ds_put_format(ds, " Auto Attach Primary Server System Id:
> > %s\n",
> > - system);
> > + id.length ? ds_cstr_ro(&system) : none_str);
> >
> > - free(id);
> > - free(system);
> > + ds_destroy(&id);
> > + ds_destroy(&system);
> > }
> > }
> > }
> > diff --git a/lib/util.c b/lib/util.c
> > index 890c606b5..fe178c995 100644
> > --- a/lib/util.c
> > +++ b/lib/util.c
> > @@ -38,6 +38,7 @@
> > #include "ovs-thread.h"
> > #include "socket-util.h"
> > #include "timeval.h"
> > +#include "openvswitch/dynamic-string.h"
> > #include "openvswitch/vlog.h"
> > #ifdef HAVE_PTHREAD_SET_NAME_NP
> > #include <pthread_np.h>
> > @@ -1077,6 +1078,18 @@ free:
> >
> > return 0;
> > }
> > +/* Converts a uint8 array to a hex string.
> > + * If delimiter is not NULL, it will be added between each hex character.
> > */
> > +void
> > +format_hex_arg(struct ds *s, const uint8_t *data, size_t len, char
> > *delimiter)
> > +{
> > + for (size_t i = 0; i < len; i++) {
> > + if (i && delimiter) {
> > + ds_put_format(s, "%s", delimiter);
> > + }
> > + ds_put_format(s, "%02" PRIx8, data[i]);
> > + }
> > +}
>
> We have a ds_put_hex and ds_put_hex_dump already, so I wonder if we
> should make this part of dynamic string library, ex:
>
> ds_put_hex_bytes_with_delimiter()
I think it's more suitable to put it in ds lib.
changed in v3.
>
> Then still do the format_hex_arg cleanup as well. WDYT?
>
> Ilya?
>
> >
> > /* Returns the current working directory as a malloc()'d string, or a null
> > * pointer if the current working directory cannot be determined. */
> > diff --git a/lib/util.h b/lib/util.h
> > index c1fd120bc..2e067107f 100644
> > --- a/lib/util.h
> > +++ b/lib/util.h
> > @@ -27,6 +27,7 @@
> > #include <stdlib.h>
> > #include <string.h>
> > #include "compiler.h"
> > +#include "openvswitch/dynamic-string.h"
> > #include "openvswitch/util.h"
> > #if defined(__aarch64__) && __GNUC__ >= 6
> > #include <arm_neon.h>
> > @@ -259,6 +260,9 @@ uintmax_t hexits_value(const char *s, size_t n, bool
> > *ok);
> > int parse_int_string(const char *s, uint8_t *valuep, int field_width,
> > char **tail);
> >
> > +void
> > +format_hex_arg(struct ds *s, const uint8_t *data, size_t len, char
> > *delimiter);
> > +
> > const char *english_list_delimiter(size_t index, size_t total);
> >
> > char *get_cwd(void);
> > diff --git a/tests/library.at b/tests/library.at
> > index 82ac80a27..6fb0a31f9 100644
> > --- a/tests/library.at
> > +++ b/tests/library.at
> > @@ -249,6 +249,10 @@ AT_KEYWORDS([sat math sat_math])
> > AT_CHECK([ovstest test-util sat_math])
> > AT_CLEANUP
> >
> > +AT_SETUP([format_hex_arg])
> > +AT_CHECK([ovstest test-util format_hex_arg])
> > +AT_CLEANUP
> > +
> > AT_SETUP([snprintf])
> > AT_CHECK([ovstest test-util snprintf])
> > AT_CLEANUP
> > diff --git a/tests/test-util.c b/tests/test-util.c
> > index 5d88d38f2..22972639a 100644
> > --- a/tests/test-util.c
> > +++ b/tests/test-util.c
> > @@ -1183,6 +1183,25 @@ test_sat_math(struct ovs_cmdl_context *ctx
> > OVS_UNUSED)
> > }
> > }
> >
> > +static void
> > +test_format_hex_arg(struct ovs_cmdl_context *ctx OVS_UNUSED)
> > +{
> > + struct ds ds = DS_EMPTY_INITIALIZER;
> > + uint8_t array[6] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06 };
> > + char str1[] = "01.02.03.04.05.06";
> > + char str2[] = "010203040506";
> > +
> > + format_hex_arg(&ds, array, 6, ".");
> > + ovs_assert(!strcmp(ds_cstr_ro(&ds), str1));
> > +
> > + ds_clear(&ds);
> > +
> > + format_hex_arg(&ds, array, 6, NULL);
> > + ovs_assert(!strcmp(ds_cstr_ro(&ds), str2));
> > +
> > + ds_destroy(&ds);
> > +}
> > +
> > #ifndef _WIN32
> > static void
> > test_file_name(struct ovs_cmdl_context *ctx)
> > @@ -1220,6 +1239,7 @@ static const struct ovs_cmdl_command commands[] = {
> > {"ovs_scan", NULL, 0, 0, test_ovs_scan, OVS_RO},
> > {"snprintf", NULL, 0, 0, test_snprintf, OVS_RO},
> > {"sat_math", NULL, 0, 0, test_sat_math, OVS_RO},
> > + {"format_hex_arg", NULL, 0, 0, test_format_hex_arg, OVS_RO},
> > #ifndef _WIN32
> > {"file_name", NULL, 1, INT_MAX, test_file_name, OVS_RO},
> > #endif
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
