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 <olivier.matz@6wind.com>
Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

diff --git a/app/test/test_hash.c b/app/test/test_hash.c
index 2f3d884ea2ae843e589b89dedef6470ec7bcd316..adbdb4a820b144f1ead3ffa0be0cacfd51087a5b 100644 (file)
--- a/app/test/test_hash.c
+++ b/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(void)
  */
 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_parameters(void)
                return -1;
        }
 
+       /* test with same name should fail */
+       memcpy(&params, &ut_params, sizeof(params));
+       params.name = "same_name";
+       handle = rte_hash_create(&params);
+       if (handle == NULL) {
+               printf("Cannot create first hash table with 'same_name'\n");
+               return -1;
+       }
+       tmp = rte_hash_create(&params);
+       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_parameters(void)
 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(&params, &ut_params, sizeof(params));
-       params.name = "same_name";
+       params.name = "name";
        params.hash_func = NULL;
        handle = rte_hash_create(&params);
        if (handle == NULL) {
@@ -1064,37 +1076,6 @@ test_hash_creation_with_good_parameters(void)
                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(&params, &ut_params, sizeof(params));
-       params.name = "same_name";
-       tmp = rte_hash_create(&params);
-
-       /* 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(&params, &ut_params, sizeof(params));
-       params.name = "different_name";
-       tmp = rte_hash_create(&params);
-       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;

diff --git a/doc/guides/rel_notes/release_16_04.rst b/doc/guides/rel_notes/release_16_04.rst
index b0f3dbabf42e05aaa3016d57be86deb5a81a1913..e2939608f05796a07b3d24c00eeb65dd420d4fcd 100644 (file)
--- a/doc/guides/rel_notes/release_16_04.rst
+++ b/doc/guides/rel_notes/release_16_04.rst
@@ -434,6 +434,15 @@ Libraries
   however a custom compare function (not in the jump table) can only
   be used in single-process mode.
 
+* **hash: Fixed return value when allocating an existing hash table.**
+
+  Changed the ``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.
+  The previous behavior was to return a pointer to the existing object in
+  that case, preventing the caller to know if the object had to be freed
+  or not.
+
 * **lpm: Fixed return value when allocating an existing object.**
 
   Changed the ``rte_lpm*_create()`` functions to return ``NULL`` and set

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 04e95b7323a2242436698ed60480537463dfec37..b00cc124282d0f5f204288f7a6eb77353fed059e 100644 (file)
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -299,8 +299,10 @@ rte_hash_create(const struct rte_hash_parameters *params)
 
        /* 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) {

diff --git a/lib/librte_hash/rte_fbk_hash.c b/lib/librte_hash/rte_fbk_hash.c
index 8752a477e9be2a854cac56d7f8926a207820e141..55c9f358319c01adf544cce3d8b6b9280128fe49 100644 (file)
--- a/lib/librte_hash/rte_fbk_hash.c
+++ b/lib/librte_hash/rte_fbk_hash.c
@@ -140,8 +140,11 @@ rte_fbk_hash_create(const struct rte_fbk_hash_params *params)
                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) {