The requested and acked seqno values are allowed to be uint64_t, however the values that were added to the hmap were truncated to uint32_t. This would lead to loss of information when the value is bigger.
Use uin64_t for the function signatures and for the hash to prevent truncation. Reported-at: https://bugzilla.redhat.com/2074019 Signed-off-by: Ales Musil <[email protected]> --- controller/ofctrl-seqno.c | 10 +++++----- controller/ofctrl-seqno.h | 2 +- controller/test-ofctrl-seqno.c | 9 +++++---- tests/ovn-ofctrl-seqno.at | 33 +++++++++++++++++++++++++++++++++ tests/test-utils.c | 17 +++++++++++++++++ tests/test-utils.h | 2 ++ 6 files changed, 63 insertions(+), 10 deletions(-) diff --git a/controller/ofctrl-seqno.c b/controller/ofctrl-seqno.c index 923de478a..95373aa57 100644 --- a/controller/ofctrl-seqno.c +++ b/controller/ofctrl-seqno.c @@ -59,7 +59,7 @@ static struct ofctrl_seqno_state *ofctrl_seqno_states; static void ofctrl_acked_seqnos_init(struct ofctrl_acked_seqnos *seqnos, uint64_t last_acked); static void ofctrl_acked_seqnos_add(struct ofctrl_acked_seqnos *seqnos, - uint32_t val); + uint64_t val); /* ofctrl_seqno_update related static function prototypes. */ static void ofctrl_seqno_update_create__(size_t seqno_type, uint64_t req_cfg); @@ -106,11 +106,11 @@ ofctrl_acked_seqnos_destroy(struct ofctrl_acked_seqnos *seqnos) /* Returns true if 'val' is one of the acked sequence numbers in 'seqnos'. */ bool ofctrl_acked_seqnos_contains(const struct ofctrl_acked_seqnos *seqnos, - uint32_t val) + uint64_t val) { struct ofctrl_ack_seqno *sn; - HMAP_FOR_EACH_WITH_HASH (sn, node, hash_int(val, 0), &seqnos->acked) { + HMAP_FOR_EACH_WITH_HASH (sn, node, hash_uint64(val), &seqnos->acked) { if (sn->seqno == val) { return true; } @@ -213,12 +213,12 @@ ofctrl_acked_seqnos_init(struct ofctrl_acked_seqnos *seqnos, } static void -ofctrl_acked_seqnos_add(struct ofctrl_acked_seqnos *seqnos, uint32_t val) +ofctrl_acked_seqnos_add(struct ofctrl_acked_seqnos *seqnos, uint64_t val) { seqnos->last_acked = val; struct ofctrl_ack_seqno *sn = xmalloc(sizeof *sn); - hmap_insert(&seqnos->acked, &sn->node, hash_int(val, 0)); + hmap_insert(&seqnos->acked, &sn->node, hash_uint64(val)); sn->seqno = val; } diff --git a/controller/ofctrl-seqno.h b/controller/ofctrl-seqno.h index 876947c26..adcc5a0d3 100644 --- a/controller/ofctrl-seqno.h +++ b/controller/ofctrl-seqno.h @@ -37,7 +37,7 @@ struct ofctrl_ack_seqno { struct ofctrl_acked_seqnos *ofctrl_acked_seqnos_get(size_t seqno_type); void ofctrl_acked_seqnos_destroy(struct ofctrl_acked_seqnos *seqnos); bool ofctrl_acked_seqnos_contains(const struct ofctrl_acked_seqnos *seqnos, - uint32_t val); + uint64_t val); void ofctrl_seqno_init(void); size_t ofctrl_seqno_add_type(void); diff --git a/controller/test-ofctrl-seqno.c b/controller/test-ofctrl-seqno.c index b96da9d2f..cd3821e04 100644 --- a/controller/test-ofctrl-seqno.c +++ b/controller/test-ofctrl-seqno.c @@ -124,9 +124,10 @@ test_ofctrl_seqno_ack_seqnos(struct ovs_cmdl_context *ctx) } for (unsigned int j = 0; j < n_app_seqnos; j++, n_reqs++) { - unsigned int app_seqno; + unsigned long long int app_seqno; - if (!test_read_uint_value(ctx, shift++, "app_seqno", &app_seqno)) { + if (!test_read_ullong_value(ctx, shift++, "app_seqno", + &app_seqno)) { return; } ofctrl_seqno_update_create(i, app_seqno); @@ -138,9 +139,9 @@ test_ofctrl_seqno_ack_seqnos(struct ovs_cmdl_context *ctx) return; } for (unsigned int i = 0; i < n_acks; i++) { - unsigned int ack_seqno; + unsigned long long int ack_seqno; - if (!test_read_uint_value(ctx, shift++, "ack_seqno", &ack_seqno)) { + if (!test_read_ullong_value(ctx, shift++, "ack_seqno", &ack_seqno)) { return; } ofctrl_seqno_run(ack_seqno); diff --git a/tests/ovn-ofctrl-seqno.at b/tests/ovn-ofctrl-seqno.at index dd7e86f10..8e3428442 100644 --- a/tests/ovn-ofctrl-seqno.at +++ b/tests/ovn-ofctrl-seqno.at @@ -223,4 +223,37 @@ ofctrl-seqno-type: 1 51 52 ]) + +AS_BOX([Ack seqno that doesn't fit in uint32_t]) +n_types=2 +n_app_seqnos=1 +app_seqnos1="4294967296" +app_seqnos2="4294967297" + +n_acks=1 +acks="1" +AT_CHECK([ovstest test-ofctrl-seqno ofctrl_seqno_ack_seqnos true ${n_types} \ + ${n_app_seqnos} ${app_seqnos1} ${n_app_seqnos} ${app_seqnos2} \ + ${n_acks} ${acks}], [0], [dnl +ofctrl-seqno-req-cfg: 2 +ofctrl-seqno-type: 0 + last-acked 4294967296 + 4294967296 +ofctrl-seqno-type: 1 + last-acked 0 +]) + +n_acks=1 +acks="2" +AT_CHECK([ovstest test-ofctrl-seqno ofctrl_seqno_ack_seqnos true ${n_types} \ + ${n_app_seqnos} ${app_seqnos1} ${n_app_seqnos} ${app_seqnos2} \ + ${n_acks} ${acks}], [0], [dnl +ofctrl-seqno-req-cfg: 2 +ofctrl-seqno-type: 0 + last-acked 4294967296 + 4294967296 +ofctrl-seqno-type: 1 + last-acked 4294967297 + 4294967297 +]) AT_CLEANUP diff --git a/tests/test-utils.c b/tests/test-utils.c index 78e63acb7..1afdc150f 100644 --- a/tests/test-utils.c +++ b/tests/test-utils.c @@ -61,3 +61,20 @@ test_read_value(struct ovs_cmdl_context *ctx, unsigned int index, return ctx->argv[index]; } + +bool +test_read_ullong_value(struct ovs_cmdl_context *ctx, unsigned int index, + const char *descr, unsigned long long int *result) +{ + if (index >= ctx->argc) { + fprintf(stderr, "Missing %s argument\n", descr); + return false; + } + + const char *arg = ctx->argv[index]; + if (!str_to_ullong(arg, 10, result)) { + fprintf(stderr, "Invalid %s: %s\n", descr, arg); + return false; + } + return true; +} diff --git a/tests/test-utils.h b/tests/test-utils.h index 910358ea1..22341cea9 100644 --- a/tests/test-utils.h +++ b/tests/test-utils.h @@ -24,5 +24,7 @@ bool test_read_uint_hex_value(struct ovs_cmdl_context *ctx, unsigned int index, const char *descr, unsigned int *result); const char *test_read_value(struct ovs_cmdl_context *ctx, unsigned int index, const char *descr); +bool test_read_ullong_value(struct ovs_cmdl_context *ctx, unsigned int index, + const char *descr, unsigned long long int *result); #endif /* tests/test-utils.h */ -- 2.40.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
