Description: backport of dpdk 16.04-rc fix for LP: #1568838 Forwarded: n/a (already upstream) Author: Christian Ehrhardt Last-Update: 2016-04-11 From 1aadacb5b0ed29ea8f84eaa3ef6802663671be98 Mon Sep 17 00:00:00 2001 From: Olivier Matz Date: Wed, 6 Apr 2016 15:27:59 +0200 Subject: [PATCH] hash: fix allocation of an existing object Change rte_hash*_create() functions to return NULL and set rte_errno to EEXIST when the object name already exists. This is the behavior described in the API documentation in the header file. These functions were returning a pointer to the existing object in that case, but it is a problem as the caller did not know if the object had to be freed or not. Doing this change also makes the hash API more consistent with the other APIs (mempool, rings, ...). Fixes: 916e4f4f4e ("memory: fix for multi process support") Signed-off-by: Olivier Matz Acked-by: Pablo de Lara --- app/test/test_hash.c | 65 ++++++++++++---------------------- doc/guides/rel_notes/release_16_04.rst | 9 +++++ lib/librte_hash/rte_cuckoo_hash.c | 6 ++-- lib/librte_hash/rte_fbk_hash.c | 5 ++- 4 files changed, 40 insertions(+), 45 deletions(-) Index: dpdk/app/test/test_hash.c =================================================================== --- dpdk.orig/app/test/test_hash.c +++ dpdk/app/test/test_hash.c @@ -805,15 +805,11 @@ fbk_hash_unit_test(void) RETURN_IF_ERROR_FBK(handle == NULL, "fbk hash creation should have succeeded"); tmp = rte_fbk_hash_create(&invalid_params_same_name_2); - RETURN_IF_ERROR_FBK(tmp == NULL, "fbk hash creation should have succeeded"); - if (tmp != handle) { - printf("ERROR line %d: hashes should have been the same\n", __LINE__); - rte_fbk_hash_free(handle); - rte_fbk_hash_free(tmp); - return -1; - } + if (tmp != NULL) + rte_fbk_hash_free(tmp); + RETURN_IF_ERROR_FBK(tmp != NULL, "fbk hash creation should have failed"); - /* we are not freeing tmp or handle here because we need a hash list + /* we are not freeing handle here because we need a hash list * to be not empty for the next test */ /* create a hash in non-empty list - good for coverage */ @@ -988,7 +984,7 @@ static int test_fbk_hash_find_existing(v */ static int test_hash_creation_with_bad_parameters(void) { - struct rte_hash *handle; + struct rte_hash *handle, *tmp; struct rte_hash_parameters params; handle = rte_hash_create(NULL); @@ -1038,7 +1034,23 @@ static int test_hash_creation_with_bad_p return -1; } + /* test with same name should fail */ + memcpy(¶ms, &ut_params, sizeof(params)); + params.name = "same_name"; + handle = rte_hash_create(¶ms); + if (handle == NULL) { + printf("Cannot create first hash table with 'same_name'\n"); + return -1; + } + tmp = rte_hash_create(¶ms); + if (tmp != NULL) { + printf("Creation of hash table with same name should fail\n"); + rte_hash_free(handle); + rte_hash_free(tmp); + return -1; + } rte_hash_free(handle); + printf("# Test successful. No more errors expected\n"); return 0; @@ -1051,12 +1063,12 @@ static int test_hash_creation_with_bad_p static int test_hash_creation_with_good_parameters(void) { - struct rte_hash *handle, *tmp; + struct rte_hash *handle; struct rte_hash_parameters params; /* create with null hash function - should choose DEFAULT_HASH_FUNC */ memcpy(¶ms, &ut_params, sizeof(params)); - params.name = "same_name"; + params.name = "name"; params.hash_func = NULL; handle = rte_hash_create(¶ms); if (handle == NULL) { @@ -1064,37 +1076,6 @@ test_hash_creation_with_good_parameters( return -1; } - /* this test is trying to create a hash with the same name as previous one. - * this should return a pointer to the hash we previously created. - * the previous hash isn't freed exactly for the purpose of it being in - * the hash list. - */ - memcpy(¶ms, &ut_params, sizeof(params)); - params.name = "same_name"; - tmp = rte_hash_create(¶ms); - - /* check if the returned handle is actually equal to the previous hash */ - if (handle != tmp) { - rte_hash_free(handle); - rte_hash_free(tmp); - printf("Creating hash with existing name was successful\n"); - return -1; - } - - /* try creating hash when there already are hashes in the list. - * the previous hash is not freed to have a non-empty hash list. - * the other hash that's in the list is still pointed to by "handle" var. - */ - memcpy(¶ms, &ut_params, sizeof(params)); - params.name = "different_name"; - tmp = rte_hash_create(¶ms); - if (tmp == NULL) { - rte_hash_free(handle); - printf("Creating hash with valid parameters failed\n"); - return -1; - } - - rte_hash_free(tmp); rte_hash_free(handle); return 0; Index: dpdk/lib/librte_hash/rte_cuckoo_hash.c =================================================================== --- dpdk.orig/lib/librte_hash/rte_cuckoo_hash.c +++ dpdk/lib/librte_hash/rte_cuckoo_hash.c @@ -300,8 +300,10 @@ rte_hash_create(const struct rte_hash_pa /* Guarantee there's no existing */ h = rte_hash_find_existing(params->name); - if (h != NULL) - return h; + if (h != NULL) { + rte_errno = EEXIST; + return NULL; + } te = rte_zmalloc("HASH_TAILQ_ENTRY", sizeof(*te), 0); if (te == NULL) { Index: dpdk/lib/librte_hash/rte_fbk_hash.c =================================================================== --- dpdk.orig/lib/librte_hash/rte_fbk_hash.c +++ dpdk/lib/librte_hash/rte_fbk_hash.c @@ -140,8 +140,11 @@ rte_fbk_hash_create(const struct rte_fbk if (strncmp(params->name, ht->name, RTE_FBK_HASH_NAMESIZE) == 0) break; } - if (te != NULL) + ht = NULL; + if (te != NULL) { + rte_errno = EEXIST; goto exit; + } te = rte_zmalloc("FBK_HASH_TAILQ_ENTRY", sizeof(*te), 0); if (te == NULL) {