[PATCH ding-libs] Extend API to const key for clients that don't need to modify their keys

[Philip Prindeville]

From: Philip Prindeville

Add c_str as a "const char *" to the hash_key_t.

Redux per comments from Michal Zidek.
---
 dhash/dhash.c               | 29 +++++++++++++++++++++--------
 dhash/dhash.h               |  4 +++-
 dhash/examples/dhash_test.c |  3 +++
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/dhash/dhash.c b/dhash/dhash.c
index 45ee0cf..98439e8 100644
--- a/dhash/dhash.c
+++ b/dhash/dhash.c
@@ -176,7 +176,7 @@ static void sys_free_wrapper(void *ptr, void *pvt)
 static address_t convert_key(hash_key_t *key)
 {
     address_t h;
-    unsigned char *k;
+    const unsigned char *k;

switch(key->type) {
     case HASH_KEY_ULONG:
@@ -184,7 +184,12 @@ static address_t convert_key(hash_key_t *key)
         break;
     case HASH_KEY_STRING:
         /* Convert string to integer */
-        for (h = 0, k = (unsigned char *) key->str; *k; k++)
+        for (h = 0, k = (const unsigned char *) key->str; *k; k++)
+            h = h * PRIME_1 ^ (*k - ' ');
+        break;
+    case HASH_KEY_CONST_STRING:
+        /* Convert string to integer */
+        for (h = 0, k = (const unsigned char *) key->c_str; *k; k++)
             h = h * PRIME_1 ^ (*k - ' ');
         break;
     default:
@@ -212,6 +217,7 @@ static bool is_valid_key_type(hash_key_enum key_type)
     switch(key_type) {
     case HASH_KEY_ULONG:
     case HASH_KEY_STRING:
+    case HASH_KEY_CONST_STRING:
         return true;
     default:
         return false;
@@ -244,6 +250,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b)
         return (a->ul == b->ul);
     case HASH_KEY_STRING:
         return (strcmp(a->str, b->str) == 0);
+    case HASH_KEY_CONST_STRING:
+        return (strcmp(a->c_str, b->c_str) == 0);
     }
     return false;
 }
@@ -670,8 +678,11 @@ int hash_destroy(hash_table_t *table)
                         while (p != NULL) {
                             q = p->next;
                             hdelete_callback(table, HASH_TABLE_DESTROY, &p->entry);
-                            if (p->entry.key.type == HASH_KEY_STRING) {
-                                hfree(table, (char *)p->entry.key.str);
+                            if (p->entry.key.type == HASH_KEY_STRING
+                                    || p->entry.key.type == HASH_KEY_CONST_STRING) {
+                                /* Internally we do not use constant memory for keys
+                                 * in hash table elements. */
+                                hfree(table, p->entry.key.str);
                             }
                             hfree(table, (char *)p);
                             p = q;
@@ -972,13 +983,14 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value)
             element->entry.key.ul = key->ul;
             break;
         case HASH_KEY_STRING:
-            len = strlen(key->str)+1;
+        case HASH_KEY_CONST_STRING:
+            len = strlen(key->c_str) + 1;
             element->entry.key.str = halloc(table, len);
             if (element->entry.key.str == NULL) {
                 hfree(table, element);
                 return HASH_ERROR_NO_MEMORY;
             }
-            memcpy((void *)element->entry.key.str, key->str, len);
+            memcpy(element->entry.key.str, key->str, len);
             break;
         }

@@ -1070,8 +1082,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key)
                 return error;
             }
         }
-        if (element->entry.key.type == HASH_KEY_STRING) {
-            hfree(table, (char *)element->entry.key.str);
+        if (element->entry.key.type == HASH_KEY_STRING
+                || element->entry.key.type == HASH_KEY_CONST_STRING) {
+            hfree(table, element->entry.key.str);
         }
         hfree(table, element);
         return HASH_SUCCESS;
diff --git a/dhash/dhash.h b/dhash/dhash.h
index baa0d6a..c36ab79 100644
--- a/dhash/dhash.h
+++ b/dhash/dhash.h
@@ -112,7 +112,8 @@ typedef struct hash_table_str hash_table_t;

typedef enum {
     HASH_KEY_STRING,
-    HASH_KEY_ULONG
+    HASH_KEY_ULONG,
+    HASH_KEY_CONST_STRING
 } hash_key_enum;

typedef enum
@@ -137,6 +138,7 @@ typedef struct hash_key_t {
     hash_key_enum type;
     union {
         char *str;
+        const char *c_str;
         unsigned long ul;
     };
 } hash_key_t;
diff --git a/dhash/examples/dhash_test.c b/dhash/examples/dhash_test.c
index 303e9f8..7613e23 100644
--- a/dhash/examples/dhash_test.c
+++ b/dhash/examples/dhash_test.c
@@ -64,6 +64,9 @@ static char *key_string(hash_key_t *key)
     case HASH_KEY_STRING:
         snprintf(buf, sizeof(buf), "key string = "%s"", key->str);
         break;
+    case HASH_KEY_CONST_STRING:
+        snprintf(buf, sizeof(buf), "key string = "%s"", key->c_str);
+        break;
     default:
         snprintf(buf, sizeof(buf), "unknown key type = %d", key->type);
         break;

[Michal Židek]

ACK to the code from Philip. I amended the commit
message to meet our style.

I would like to push this together with at least some
sanity tests. See the second patch. I am looking for
someone from SSSD developers to review the tests.

Michal

PS: Btw. Philip, I am interested for what project are you
using ding-libs.

On 09/30/2016 11:55 PM, Philip Prindeville wrote:
> From: Philip Prindeville
>
> Add c_str as a "const char *" to the hash_key_t.
> Redux per comments from Michal Zidek.
>
> dhash/dhash.c               | 29 +++++++++++++++++++++--------
>   dhash/dhash.h               |  4 +++-
>   dhash/examples/dhash_test.c |  3 +++
>   3 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/dhash/dhash.c b/dhash/dhash.c
> index 45ee0cf..98439e8 100644
> --- a/dhash/dhash.c
> +++ b/dhash/dhash.c
> @@ -176,7 +176,7 @@ static void sys_free_wrapper(void *ptr, void *pvt)
>   static address_t convert_key(hash_key_t *key)
>   {
>       address_t h;
> unsigned char *k;
>
> const unsigned char *k;
>
> switch(key->type) {
>  case HASH_KEY_ULONG:
>
> @@ -184,7 +184,12 @@ static address_t convert_key(hash_key_t *key)
>           break;
>       case HASH_KEY_STRING:
>           /* Convert string to integer */
>    for (h = 0, k = (unsigned char *) key->str; *k; k++)
>    for (h = 0, k = (const unsigned char *) key->str; *k; k++)
>        h = h * PRIME_1 ^ (*k - ' ');
>    break;
> case HASH_KEY_CONST_STRING:
>    /* Convert string to integer */
>    for (h = 0, k = (const unsigned char *) key->c_str; *k; k++)
>         h = h * PRIME_1 ^ (*k - ' ');
>     break;
> default:
>
> @@ -212,6 +217,7 @@ static bool is_valid_key_type(hash_key_enum key_type)
>       switch(key_type) {
>       case HASH_KEY_ULONG:
>       case HASH_KEY_STRING:
> case HASH_KEY_CONST_STRING:
>      return true;
>  default:
>      return false;
>
> @@ -244,6 +250,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b)
>           return (a->ul == b->ul);
>       case HASH_KEY_STRING:
>           return (strcmp(a->str, b->str) == 0);
> case HASH_KEY_CONST_STRING:
>    return (strcmp(a->c_str, b->c_str) == 0);
> }
> return false;
> }
>
> @@ -670,8 +678,11 @@ int hash_destroy(hash_table_t *table)
>                           while (p != NULL) {
>                               q = p->next;
>                               hdelete_callback(table, HASH_TABLE_DESTROY, &p->entry);
>                        if (p->entry.key.type == HASH_KEY_STRING) {
>                            hfree(table, (char *)p->entry.key.str);
>                        if (p->entry.key.type == HASH_KEY_STRING
>                                || p->entry.key.type == HASH_KEY_CONST_STRING) {
>                            /* Internally we do not use constant memory for keys
>                             * in hash table elements. */
>                            hfree(table, p->entry.key.str);
>                         }
>                         hfree(table, (char *)p);
>                         p = q;
>
> @@ -972,13 +983,14 @@ int hash_enter(hash_table_t *table, hash_key_t *key, hash_value_t *value)
>               element->entry.key.ul = key->ul;
>               break;
>           case HASH_KEY_STRING:
>        len = strlen(key->str)+1;
>    case HASH_KEY_CONST_STRING:
>        len = strlen(key->c_str) + 1;
>         element->entry.key.str = halloc(table, len);
>         if (element->entry.key.str == NULL) {
>             hfree(table, element);
>             return HASH_ERROR_NO_MEMORY;
>         }
>        memcpy((void *)element->entry.key.str, key->str, len);
>        memcpy(element->entry.key.str, key->str, len);
>         break;
>     }
>
> @@ -1070,8 +1082,9 @@ int hash_delete(hash_table_t *table, hash_key_t *key)
>                   return error;
>               }
>           }
>    if (element->entry.key.type == HASH_KEY_STRING) {
>        hfree(table, (char *)element->entry.key.str);
>    if (element->entry.key.type == HASH_KEY_STRING
>            || element->entry.key.type == HASH_KEY_CONST_STRING) {
>        hfree(table, element->entry.key.str);
>     }
>     hfree(table, element);
>     return HASH_SUCCESS;
>
> diff --git a/dhash/dhash.h b/dhash/dhash.h
> index baa0d6a..c36ab79 100644
> --- a/dhash/dhash.h
> +++ b/dhash/dhash.h
> @@ -112,7 +112,8 @@ typedef struct hash_table_str hash_table_t;
>
> typedef enum {
>       HASH_KEY_STRING,
> HASH_KEY_ULONG
>
> HASH_KEY_ULONG,
>
> HASH_KEY_CONST_STRING
> } hash_key_enum;
>
> typedef enum
>
> @@ -137,6 +138,7 @@ typedef struct hash_key_t {
>       hash_key_enum type;
>       union {
>           char *str;
>    const char *c_str;
>     unsigned long ul;
> };
> } hash_key_t;
>
> diff --git a/dhash/examples/dhash_test.c b/dhash/examples/dhash_test.c
> index 303e9f8..7613e23 100644
> --- a/dhash/examples/dhash_test.c
> +++ b/dhash/examples/dhash_test.c
> @@ -64,6 +64,9 @@ static char *key_string(hash_key_t *key)
>       case HASH_KEY_STRING:
>           snprintf(buf, sizeof(buf), "key string = "%s"", key->str);
>           break;
> case HASH_KEY_CONST_STRING:
>    snprintf(buf, sizeof(buf), "key string = \"%s\"", key->c_str);
>    break;
> default:
>     snprintf(buf, sizeof(buf), "unknown key type = %d", key->type);
>     break;

[Michal Židek]

I forgot to attach the patches.

Again the first one is acked by me, the second
needs a review.

Michal

On 10/05/2016 01:45 PM, Michal Židek wrote:
> ACK to the code from Philip. I amended the commit
> message to meet our style.
>
> I would like to push this together with at least some
> sanity tests. See the second patch. I am looking for
> someone from SSSD developers to review the tests.
>
> Michal
>
> PS: Btw. Philip, I am interested for what project are you
> using ding-libs.
>
> On 09/30/2016 11:55 PM, Philip Prindeville wrote:
> > From: Philip Prindeville
> >
> > Add c_str as a "const char *" to the hash_key_t.
> > Redux per comments from Michal Zidek.
> >
> > dhash/dhash.c               | 29 +++++++++++++++++++++--------
> >   dhash/dhash.h               |  4 +++-
> >   dhash/examples/dhash_test.c |  3 +++
> >   3 files changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/dhash/dhash.c b/dhash/dhash.c
> > index 45ee0cf..98439e8 100644
> > --- a/dhash/dhash.c
> > +++ b/dhash/dhash.c
> > @@ -176,7 +176,7 @@ static void sys_free_wrapper(void *ptr, void *pvt)
> >   static address_t convert_key(hash_key_t *key)
> >   {
> >       address_t h;
> > unsigned char *k;
> >
> > const unsigned char *k;
> >
> > switch(key->type) {
> >  case HASH_KEY_ULONG:
> >
> > @@ -184,7 +184,12 @@ static address_t convert_key(hash_key_t *key)
> >           break;
> >       case HASH_KEY_STRING:
> >           /* Convert string to integer */
> >    for (h = 0, k = (unsigned char *) key->str; *k; k++)
> >    for (h = 0, k = (const unsigned char *) key->str; *k; k++)
> >        h = h * PRIME_1 ^ (*k - ' ');
> >    break;
> > case HASH_KEY_CONST_STRING:
> >    /* Convert string to integer */
> >    for (h = 0, k = (const unsigned char *) key->c_str; *k; k++)
> >         h = h * PRIME_1 ^ (*k - ' ');
> >     break;
> > default:
> >
> > @@ -212,6 +217,7 @@ static bool is_valid_key_type(hash_key_enum key_type)
> >       switch(key_type) {
> >       case HASH_KEY_ULONG:
> >       case HASH_KEY_STRING:
> > case HASH_KEY_CONST_STRING:
> >      return true;
> >  default:
> >      return false;
> >
> > @@ -244,6 +250,8 @@ static bool key_equal(hash_key_t *a, hash_key_t *b)
> >           return (a->ul == b->ul);
> >       case HASH_KEY_STRING:
> >           return (strcmp(a->str, b->str) == 0);
> > case HASH_KEY_CONST_STRING:
> >    return (strcmp(a->c_str, b->c_str) == 0);
> > }
> > return false;
> > }
> >
> > @@ -670,8 +678,11 @@ int hash_destroy(hash_table_t *table)
> >                           while (p != NULL) {
> >                               q = p->next;
> >                               hdelete_callback(table,
> > HASH_TABLE_DESTROY, &p->entry);
> >                        if (p->entry.key.type == HASH_KEY_STRING) {
> >                            hfree(table, (char *)p->entry.key.str);
> >                        if (p->entry.key.type == HASH_KEY_STRING
> >                                || p->entry.key.type ==
> >
> > HASH_KEY_CONST_STRING) {
> >                            /* Internally we do not use constant
> >
> > memory for keys
> >                             * in hash table elements. */
> >                            hfree(table, p->entry.key.str);
> >                         }
> >                         hfree(table, (char *)p);
> >                         p = q;
> >
> > @@ -972,13 +983,14 @@ int hash_enter(hash_table_t *table, hash_key_t
> > *key, hash_value_t *value)
> >               element->entry.key.ul = key->ul;
> >               break;
> >           case HASH_KEY_STRING:
> >        len = strlen(key->str)+1;
> >    case HASH_KEY_CONST_STRING:
> >        len = strlen(key->c_str) + 1;
> >         element->entry.key.str = halloc(table, len);
> >         if (element->entry.key.str == NULL) {
> >             hfree(table, element);
> >             return HASH_ERROR_NO_MEMORY;
> >         }
> >        memcpy((void *)element->entry.key.str, key->str, len);
> >        memcpy(element->entry.key.str, key->str, len);
> >         break;
> >     }
> >
> > @@ -1070,8 +1082,9 @@ int hash_delete(hash_table_t *table, hash_key_t
> > *key)
> >                   return error;
> >               }
> >           }
> >    if (element->entry.key.type == HASH_KEY_STRING) {
> >        hfree(table, (char *)element->entry.key.str);
> >    if (element->entry.key.type == HASH_KEY_STRING
> >            || element->entry.key.type == HASH_KEY_CONST_STRING) {
> >        hfree(table, element->entry.key.str);
> >     }
> >     hfree(table, element);
> >     return HASH_SUCCESS;
> >
> > diff --git a/dhash/dhash.h b/dhash/dhash.h
> > index baa0d6a..c36ab79 100644
> > --- a/dhash/dhash.h
> > +++ b/dhash/dhash.h
> > @@ -112,7 +112,8 @@ typedef struct hash_table_str hash_table_t;
> >
> > typedef enum {
> >       HASH_KEY_STRING,
> > HASH_KEY_ULONG
> >
> > HASH_KEY_ULONG,
> >
> > HASH_KEY_CONST_STRING
> > } hash_key_enum;
> >
> > typedef enum
> >
> > @@ -137,6 +138,7 @@ typedef struct hash_key_t {
> >       hash_key_enum type;
> >       union {
> >           char *str;
> >    const char *c_str;
> >     unsigned long ul;
> > };
> > } hash_key_t;
> >
> > diff --git a/dhash/examples/dhash_test.c b/dhash/examples/dhash_test.c
> > index 303e9f8..7613e23 100644
> > --- a/dhash/examples/dhash_test.c
> > +++ b/dhash/examples/dhash_test.c
> > @@ -64,6 +64,9 @@ static char *key_string(hash_key_t *key)
> >       case HASH_KEY_STRING:
> >           snprintf(buf, sizeof(buf), "key string = "%s"", key->str);
> >           break;
> > case HASH_KEY_CONST_STRING:
> >    snprintf(buf, sizeof(buf), "key string = \"%s\"", key->c_str);
> >    break;
> > default:
> >     snprintf(buf, sizeof(buf), "unknown key type = %d", key->type);
> >     break;
>
> sssd-devel mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[Philip Prindeville]

> On Oct 5, 2016, at 7:18 AM, Michal Židek  wrote:
>
> I forgot to attach the patches.
>
> Again the first one is acked by me, the second
> needs a review.
>
> Michal

Thanks for writing those tests.

Minor comment, dhash_ut_check.c and the existing checks don’t have any negative tests, such as attempting to delete a non-existent key, or deleting an already deleted key…  or entering a key which is already present.

-Philip

[Michal Židek]

On 10/05/2016 03:47 PM, Philip Prindeville wrote:
> > On Oct 5, 2016, at 7:18 AM, Michal Židek  wrote:
> >
> > I forgot to attach the patches.
> >
> > Again the first one is acked by me, the second
> > needs a review.
> >
> > Michal
>
> Thanks for writing those tests.
>
> Minor comment, dhash_ut_check.c and the existing checks don’t have any negative tests, such as attempting to delete a non-existent key, or deleting an already deleted key…  or entering a key which is already present.
>
> -Philip

Good point. I added some delete operations to these tests.
However these are just some sanity tests to cover the code that
was changed. My intention was not to test everything here.

New tests are attached.

Michal

[Petr Cech]

On 10/05/2016 04:18 PM, Michal Židek wrote:
> On 10/05/2016 03:47 PM, Philip Prindeville wrote:
> > > On Oct 5, 2016, at 7:18 AM, Michal Židek  wrote:
> > >
> > > I forgot to attach the patches.
> > >
> > > Again the first one is acked by me, the second
> > > needs a review.
> > >
> > > Michal
> >
> > Thanks for writing those tests.
> >
> > Minor comment, dhash_ut_check.c and the existing checks don’t have any
> > negative tests, such as attempting to delete a non-existent key, or
> > deleting an already deleted key…  or entering a key which is already
> > present.
> >
> > -Philip
>
> Good point. I added some delete operations to these tests.
> However these are just some sanity tests to cover the code that
> was changed. My intention was not to test everything here.
>
> New tests are attached.
>
> Michal

Hello Michal,

I comment two things online.
> 0002-DHASH-Add-check-based-unit-test.patch
>
> From 83b18c3ca4d70086bbdc645e0d09e7e027e5e9b3 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= 
> Date: Wed, 5 Oct 2016 13:10:35 +0200
> Subject: [PATCH 2/2] DHASH: Add check based unit test
>
> Makefile.am            |  14 ++++
>  dhash/dhash_ut_check.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 224 insertions(+)
>  create mode 100644 dhash/dhash_ut_check.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 65528a8..ca9710e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -114,12 +114,26 @@ libdhash_la_LDFLAGS = \
>  check_PROGRAMS += dhash_test dhash_example
>  TESTS += dhash_test dhash_example
>
> +if HAVE_CHECK
> check_PROGRAMS += dhash_ut_check
> TESTS += dhash_ut_check
>
> +endif
>
> dhash_test_SOURCES = dhash/examples/dhash_test.c
>  dhash_test_LDADD = libdhash.la
>
> dhash_example_SOURCES = dhash/examples/dhash_example.c
>  dhash_example_LDADD = libdhash.la
>
> +dhash_ut_check_SOURCES = dhash/dhash_ut_check.c
> +dhash_ut_chech_CFLAGS = $(AM_CFLAGS) \
>                    $(CHECK_CFLAGS) \
>                    $(NULL)
>
> +dhash_ut_check_LDADD = libdhash.la \
>                   $(CHECK_LIBS) \
>                   $(NULL)
>
> dist_examples_DATA += \
>      dhash/examples/dhash_test.c \
>      dhash/examples/dhash_example.c
> diff --git a/dhash/dhash_ut_check.c b/dhash/dhash_ut_check.c
> new file mode 100644
> index 0000000..b4b36fa
> --- /dev/null
> +++ b/dhash/dhash_ut_check.c
> @@ -0,0 +1,210 @@
> +/*
> Authors:
>    Michal Zidek <[e-mail]>
> Copyright (C) 2016 Red Hat
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU Lesser General Public License as published by
> the Free Software Foundation; either version 3 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU Lesser General Public License for more details.
> You should have received a copy of the GNU Lesser General Public License
> along with this program.  If not, see http://www.gnu.org/licenses/.
>
> +*/
>
> +#include "config.h"
>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <string.h>

IMO, this is unnecessary.
> +#include <stdlib.h>
> +#include <check.h>
>
> +/* #define TRACE_LEVEL 7 */
> +#define TRACE_HOME
> +#include "dhash.h"
> +#include "path_utils.h"

IMO, this is unnecessary.
> +#define HTABLE_SIZE 128
>
> +int verbose = 0;
>
> +/* There must be no warnings generated during this test
> without having to cast the key value. */
>
> +START_TEST(test_key_const_string)
> +{
> hash_table_t *htable;
> int ret;
> hash_value_t ret_val;
> hash_value_t enter_val1 = {.type = HASH_VALUE_INT, .i = 1};
> hash_value_t enter_val2 = {.type = HASH_VALUE_INT, .i = 2};
> hash_key_t key = {.type = HASH_KEY_CONST_STRING, .c_str = "constant"};
> ret = hash_create(HTABLE_SIZE, &htable, NULL, NULL);
> fail_unless(ret == 0);
> /* The table is empty, lookup should return error */
> ret = hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> /* Deleting with non-existing key should return error */
> ret = hash_delete(htable, &key);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> ret = hash_enter(htable, &key, &enter_val1);
> fail_unless(ret == 0);
> hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == 0);
> fail_unless(ret_val.i == 1);
> /* Overwrite the entry */
> ret = hash_enter(htable, &key, &enter_val2);
> fail_unless(ret == 0);
> hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == 0);
> fail_unless(ret_val.i == 2);
> ret = hash_delete(htable, &key);
> fail_unless(ret == 0);
> /* Delete again with the same key */
> ret = hash_delete(htable, &key);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> ret = hash_destroy(htable);
> fail_unless(ret == 0);
>
> +}
> +END_TEST
>
> +START_TEST(test_key_string)
> +{
> hash_table_t *htable;
> int ret;
> hash_value_t ret_val;
> hash_value_t enter_val1 = {.type = HASH_VALUE_INT, .i = 1};
> hash_value_t enter_val2 = {.type = HASH_VALUE_INT, .i = 2};
> hash_key_t key;
> char str[] = "non_constant";
> key.type = HASH_KEY_STRING;
> key.str = str;
> ret = hash_create(HTABLE_SIZE, &htable, NULL, NULL);
> fail_unless(ret == 0);
> /* The table is empty, lookup should return error */
> ret = hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> /* Deleting with non-existing key should return error */
> ret = hash_delete(htable, &key);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> ret = hash_enter(htable, &key, &enter_val1);
> fail_unless(ret == 0);
> hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == 0);
> fail_unless(ret_val.i == 1);
> /* Overwrite the entry */
> ret = hash_enter(htable, &key, &enter_val2);
> fail_unless(ret == 0);
> hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == 0);
> fail_unless(ret_val.i == 2);
> ret = hash_delete(htable, &key);
> fail_unless(ret == 0);
> /* Delete again with the same key */
> ret = hash_delete(htable, &key);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> ret = hash_destroy(htable);
> fail_unless(ret == 0);
>
> +}
> +END_TEST
>
> +START_TEST(test_key_ulong)
> +{
> hash_table_t *htable;
> int ret;
> hash_value_t ret_val;
> hash_value_t enter_val1 = {.type = HASH_VALUE_INT, .i = 1};
> hash_value_t enter_val2 = {.type = HASH_VALUE_INT, .i = 2};
> hash_key_t key = {.type = HASH_KEY_ULONG, .ul = 68ul};
> ret = hash_create(HTABLE_SIZE, &htable, NULL, NULL);
> fail_unless(ret == 0);
> /* The table is empty, lookup should return error */
> ret = hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> /* Deleting with non-existing key should return error */
> ret = hash_delete(htable, &key);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> ret = hash_enter(htable, &key, &enter_val1);
> fail_unless(ret == 0);
> hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == 0);
> fail_unless(ret_val.i == 1);
> /* Overwrite the entry */
> ret = hash_enter(htable, &key, &enter_val2);
> fail_unless(ret == 0);
> hash_lookup(htable, &key, &ret_val);
> fail_unless(ret == 0);
> fail_unless(ret_val.i == 2);
> ret = hash_delete(htable, &key);
> fail_unless(ret == 0);
> /* Delete again with the same key */
> ret = hash_delete(htable, &key);
> fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> ret = hash_destroy(htable);
> fail_unless(ret == 0);
>
> +}
> +END_TEST
>
> +static Suite *dhash_suite(void)
> +{
> Suite *s = suite_create("");
> TCase *tc_basic = tcase_create("dhash API tests");
> tcase_add_test(tc_basic, test_key_const_string);
> tcase_add_test(tc_basic, test_key_string);
> tcase_add_test(tc_basic, test_key_ulong);
> suite_add_tcase(s, tc_basic);
> return s;
>
> +}
>
> +int main(void)
> +{
> int number_failed;
> Suite *s = dhash_suite();
> SRunner *sr = srunner_create(s);
> /* If CK_VERBOSITY is set, use that, otherwise it defaults to CK_NORMAL */
> srunner_run_all(sr, CK_ENV);
> number_failed = srunner_ntests_failed(sr);
> srunner_free(sr);
> return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
>
> +}
> -- 1.8.3.1

[Michal Židek]

On 10/05/2016 04:30 PM, Petr Cech wrote:
> On 10/05/2016 04:18 PM, Michal Židek wrote:
> > On 10/05/2016 03:47 PM, Philip Prindeville wrote:
> > > > On Oct 5, 2016, at 7:18 AM, Michal Židek  wrote:
> > > >
> > > > I forgot to attach the patches.
> > > >
> > > > Again the first one is acked by me, the second
> > > > needs a review.
> > > >
> > > > Michal
> > >
> > > Thanks for writing those tests.
> > >
> > > Minor comment, dhash_ut_check.c and the existing checks don’t have any
> > > negative tests, such as attempting to delete a non-existent key, or
> > > deleting an already deleted key…  or entering a key which is already
> > > present.
> > >
> > > -Philip
> >
> > Good point. I added some delete operations to these tests.
> > However these are just some sanity tests to cover the code that
> > was changed. My intention was not to test everything here.
> >
> > New tests are attached.
> >
> > Michal
>
> Hello Michal,
>
> I comment two things online.

I agree with the comments. New test attached.
> > 0002-DHASH-Add-check-based-unit-test.patch
> >
> > From 83b18c3ca4d70086bbdc645e0d09e7e027e5e9b3 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Michal=20=C5=BDidek?= 
> > Date: Wed, 5 Oct 2016 13:10:35 +0200
> > Subject: [PATCH 2/2] DHASH: Add check based unit test
> >
> > Makefile.am            |  14 ++++
> >  dhash/dhash_ut_check.c | 210
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 224 insertions(+)
> >  create mode 100644 dhash/dhash_ut_check.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 65528a8..ca9710e 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -114,12 +114,26 @@ libdhash_la_LDFLAGS = \
> >  check_PROGRAMS += dhash_test dhash_example
> >  TESTS += dhash_test dhash_example
> >
> > +if HAVE_CHECK
> > check_PROGRAMS += dhash_ut_check
> > TESTS += dhash_ut_check
> >
> > +endif
> >
> > dhash_test_SOURCES = dhash/examples/dhash_test.c
> >  dhash_test_LDADD = libdhash.la
> >
> > dhash_example_SOURCES = dhash/examples/dhash_example.c
> >  dhash_example_LDADD = libdhash.la
> >
> > +dhash_ut_check_SOURCES = dhash/dhash_ut_check.c
> > +dhash_ut_chech_CFLAGS = $(AM_CFLAGS) \
> >                    $(CHECK_CFLAGS) \
> >                    $(NULL)
> >
> > +dhash_ut_check_LDADD = libdhash.la \
> >                   $(CHECK_LIBS) \
> >                   $(NULL)
> >
> > dist_examples_DATA += \
> >      dhash/examples/dhash_test.c \
> >      dhash/examples/dhash_example.c
> > diff --git a/dhash/dhash_ut_check.c b/dhash/dhash_ut_check.c
> > new file mode 100644
> > index 0000000..b4b36fa
> > --- /dev/null
> > +++ b/dhash/dhash_ut_check.c
> > @@ -0,0 +1,210 @@
> > +/*
> > Authors:
> >    Michal Zidek <[e-mail]>
> > Copyright (C) 2016 Red Hat
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU Lesser General Public License as
> >
> > published by
> > the Free Software Foundation; either version 3 of the License, or
> > (at your option) any later version.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU Lesser General Public License for more details.
> > You should have received a copy of the GNU Lesser General Public
> >
> > License
> > along with this program.  If not, see
> >
> > http://www.gnu.org/licenses/.
> > +*/
> >
> > +#include "config.h"
> >
> > +#include <errno.h>
> > +#include <stdio.h>
> > +#include <string.h>
>
> IMO, this is unnecessary.
> > +#include <stdlib.h>
> > +#include <check.h>
> >
> > +/* #define TRACE_LEVEL 7 */
> > +#define TRACE_HOME
> > +#include "dhash.h"
> > +#include "path_utils.h"
>
> IMO, this is unnecessary.
> > +#define HTABLE_SIZE 128
> >
> > +int verbose = 0;
> >
> > +/* There must be no warnings generated during this test
> > without having to cast the key value. */
> >
> > +START_TEST(test_key_const_string)
> > +{
> > hash_table_t *htable;
> > int ret;
> > hash_value_t ret_val;
> > hash_value_t enter_val1 = {.type = HASH_VALUE_INT, .i = 1};
> > hash_value_t enter_val2 = {.type = HASH_VALUE_INT, .i = 2};
> > hash_key_t key = {.type = HASH_KEY_CONST_STRING, .c_str =
> >
> > "constant"};
> > ret = hash_create(HTABLE_SIZE, &htable, NULL, NULL);
> > fail_unless(ret == 0);
> > /* The table is empty, lookup should return error */
> > ret = hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > /* Deleting with non-existing key should return error */
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > ret = hash_enter(htable, &key, &enter_val1);
> > fail_unless(ret == 0);
> > hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == 0);
> > fail_unless(ret_val.i == 1);
> > /* Overwrite the entry */
> > ret = hash_enter(htable, &key, &enter_val2);
> > fail_unless(ret == 0);
> > hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == 0);
> > fail_unless(ret_val.i == 2);
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == 0);
> > /* Delete again with the same key */
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > ret = hash_destroy(htable);
> > fail_unless(ret == 0);
> >
> > +}
> > +END_TEST
> >
> > +START_TEST(test_key_string)
> > +{
> > hash_table_t *htable;
> > int ret;
> > hash_value_t ret_val;
> > hash_value_t enter_val1 = {.type = HASH_VALUE_INT, .i = 1};
> > hash_value_t enter_val2 = {.type = HASH_VALUE_INT, .i = 2};
> > hash_key_t key;
> > char str[] = "non_constant";
> > key.type = HASH_KEY_STRING;
> > key.str = str;
> > ret = hash_create(HTABLE_SIZE, &htable, NULL, NULL);
> > fail_unless(ret == 0);
> > /* The table is empty, lookup should return error */
> > ret = hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > /* Deleting with non-existing key should return error */
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > ret = hash_enter(htable, &key, &enter_val1);
> > fail_unless(ret == 0);
> > hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == 0);
> > fail_unless(ret_val.i == 1);
> > /* Overwrite the entry */
> > ret = hash_enter(htable, &key, &enter_val2);
> > fail_unless(ret == 0);
> > hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == 0);
> > fail_unless(ret_val.i == 2);
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == 0);
> > /* Delete again with the same key */
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > ret = hash_destroy(htable);
> > fail_unless(ret == 0);
> >
> > +}
> > +END_TEST
> >
> > +START_TEST(test_key_ulong)
> > +{
> > hash_table_t *htable;
> > int ret;
> > hash_value_t ret_val;
> > hash_value_t enter_val1 = {.type = HASH_VALUE_INT, .i = 1};
> > hash_value_t enter_val2 = {.type = HASH_VALUE_INT, .i = 2};
> > hash_key_t key = {.type = HASH_KEY_ULONG, .ul = 68ul};
> > ret = hash_create(HTABLE_SIZE, &htable, NULL, NULL);
> > fail_unless(ret == 0);
> > /* The table is empty, lookup should return error */
> > ret = hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > /* Deleting with non-existing key should return error */
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > ret = hash_enter(htable, &key, &enter_val1);
> > fail_unless(ret == 0);
> > hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == 0);
> > fail_unless(ret_val.i == 1);
> > /* Overwrite the entry */
> > ret = hash_enter(htable, &key, &enter_val2);
> > fail_unless(ret == 0);
> > hash_lookup(htable, &key, &ret_val);
> > fail_unless(ret == 0);
> > fail_unless(ret_val.i == 2);
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == 0);
> > /* Delete again with the same key */
> > ret = hash_delete(htable, &key);
> > fail_unless(ret == HASH_ERROR_KEY_NOT_FOUND);
> > ret = hash_destroy(htable);
> > fail_unless(ret == 0);
> >
> > +}
> > +END_TEST
> >
> > +static Suite *dhash_suite(void)
> > +{
> > Suite *s = suite_create("");
> > TCase *tc_basic = tcase_create("dhash API tests");
> > tcase_add_test(tc_basic, test_key_const_string);
> > tcase_add_test(tc_basic, test_key_string);
> > tcase_add_test(tc_basic, test_key_ulong);
> > suite_add_tcase(s, tc_basic);
> > return s;
> >
> > +}
> >
> > +int main(void)
> > +{
> > int number_failed;
> > Suite *s = dhash_suite();
> > SRunner *sr = srunner_create(s);
> > /* If CK_VERBOSITY is set, use that, otherwise it defaults to
> >
> > CK_NORMAL */
> > srunner_run_all(sr, CK_ENV);
> > number_failed = srunner_ntests_failed(sr);
> > srunner_free(sr);
> > return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
> >
> > +}
> > -- 1.8.3.1

[Petr Cech]

On 10/05/2016 04:39 PM, Michal Židek wrote:
> On 10/05/2016 04:30 PM, Petr Cech wrote:
> > On 10/05/2016 04:18 PM, Michal Židek wrote:
> > > On 10/05/2016 03:47 PM, Philip Prindeville wrote:
> > > > > On Oct 5, 2016, at 7:18 AM, Michal Židek  wrote:
> > Hello Michal,
> >
> > I comment two things online.
>
> I agree with the comments. New test attached.
> > > 0002-DHASH-Add-check-based-unit-test.patch
> > >
> > > From 83b18c3ca4d70086bbdc645e0d09e7e027e5e9b3 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Michal=20=C5=BDidek?= 
> > > Date: Wed, 5 Oct 2016 13:10:35 +0200
> > > Subject: [PATCH 2/2] DHASH: Add check based unit test

LGTM.
ACK.

Regards

[Michal Židek]

On 10/05/2016 04:48 PM, Petr Cech wrote:
> On 10/05/2016 04:39 PM, Michal Židek wrote:
> > On 10/05/2016 04:30 PM, Petr Cech wrote:
> > > On 10/05/2016 04:18 PM, Michal Židek wrote:
> > > > On 10/05/2016 03:47 PM, Philip Prindeville wrote:
> > > > > > On Oct 5, 2016, at 7:18 AM, Michal Židek  wrote:
> > > Hello Michal,
> > >
> > > I comment two things online.
> >
> > I agree with the comments. New test attached.
> > > > 0002-DHASH-Add-check-based-unit-test.patch
> > > >
> > > > From 83b18c3ca4d70086bbdc645e0d09e7e027e5e9b3 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Michal=20=C5=BDidek?= 
> > > > Date: Wed, 5 Oct 2016 13:10:35 +0200
> > > > Subject: [PATCH 2/2] DHASH: Add check based unit test
>
> LGTM.
> ACK.
>
> Regards

Pushed to ding-libs master:
* d713c1f979ba3e046a6dd53f5262ae0cdbe53bc4
* f085b9b26f6e00cec4a0acf6896843436234ca4f

Michal

[Philip Prindeville]

On Oct 5, 2016, at 5:45 AM, Michal Židek  wrote:
> ACK to the code from Philip. I amended the commit
> message to meet our style.
>
> I would like to push this together with at least some
> sanity tests. See the second patch. I am looking for
> someone from SSSD developers to review the tests.
>
> Michal
>
> PS: Btw. Philip, I am interested for what project are you
> using ding-libs.

The project was an SSSD provider (proxy) for Tacacs+ accounting, authentication, and authorization.

-Philip

[Jakub Hrozek]

On Wed, Oct 05, 2016 at 07:42:23AM -0600, Philip Prindeville wrote:
> On Oct 5, 2016, at 5:45 AM, Michal Židek  wrote:
> > ACK to the code from Philip. I amended the commit
> > message to meet our style.
> >
> > I would like to push this together with at least some
> > sanity tests. See the second patch. I am looking for
> > someone from SSSD developers to review the tests.
> >
> > Michal
> >
> > PS: Btw. Philip, I am interested for what project are you
> > using ding-libs.
>
> The project was an SSSD provider (proxy) for Tacacs+ accounting, authentication, and authorization.

Wow, I'm intrigued and I think all of us want to help!

Is that an in-house project or do you plan on open sourcing it? If the
latter, I wonder if we could help with review, writing more docs or just
about anything?

[Michal Židek]

On 10/05/2016 05:43 PM, Jakub Hrozek wrote:
> On Wed, Oct 05, 2016 at 07:42:23AM -0600, Philip Prindeville wrote:
> > On Oct 5, 2016, at 5:45 AM, Michal Židek  wrote:
> > > ACK to the code from Philip. I amended the commit
> > > message to meet our style.
> > >
> > > I would like to push this together with at least some
> > > sanity tests. See the second patch. I am looking for
> > > someone from SSSD developers to review the tests.
> > >
> > > Michal
> > >
> > > PS: Btw. Philip, I am interested for what project are you
> > > using ding-libs.
> >
> > The project was an SSSD provider (proxy) for Tacacs+ accounting, authentication, and authorization.
>
> Wow, I'm intrigued and I think all of us want to help!
>
> Is that an in-house project or do you plan on open sourcing it? If the
> latter, I wonder if we could help with review, writing more docs or just
> about anything?

I agree that this looks interesting. It would be good if
the discussion about it took place here on the sssd devel-list,
and maybe we could give a hand with something.

Michal

[Philip Prindeville]

> On Oct 5, 2016, at 9:43 AM, Jakub Hrozek  wrote:
>
> On Wed, Oct 05, 2016 at 07:42:23AM -0600, Philip Prindeville wrote:
> > On Oct 5, 2016, at 5:45 AM, Michal Židek  wrote:
> > > ACK to the code from Philip. I amended the commit
> > > message to meet our style.
> > >
> > > I would like to push this together with at least some
> > > sanity tests. See the second patch. I am looking for
> > > someone from SSSD developers to review the tests.
> > >
> > > Michal
> > >
> > > PS: Btw. Philip, I am interested for what project are you
> > > using ding-libs.
> >
> > The project was an SSSD provider (proxy) for Tacacs+ accounting, authentication, and authorization.
>
> Wow, I'm intrigued and I think all of us want to help!
>
> Is that an in-house project or do you plan on open sourcing it? If the
> latter, I wonder if we could help with review, writing more docs or just
> about anything?

Sure, we were planning on upstreaming it.

For the Tacacs+ part, I had used an event-driven dispatcher (libevent), but the DBus listener ran in a separate thread…

I had wanted to make everything be event-driven, including servicing the arrival of DBus requests from SSSD, because then you could handle request/reply pairs asynchronously for Tacacs, which then enables you to let Tacacs+ reuse connections to servers if they’re already up, or do multiplexing (i.e. having multiple requests on the fly, and having responses come back out of order, etc).

That part I haven’t figured out yet… how to integrate DBus with libevent.  There are a couple of posting online about how to do this, but nothing that looks very elegant.  See:

https://lists.freedesktop.org/archives/dbus/2016-August/017005.html https://lists.freedesktop.org/archives/dbus/2016-August/017005.html

The one example I saw (http://stackoverflow.com/questions/9378593/dbuswatch-and-dbustimeout-example...) struck me as having an unfortunate race condition.  I looked at that and thought, “there must be a better way”.

So once I clear that hurdle, and the changes I had to make to pam_tacplus’s libtac library get accepted upstream (I’m about a third of the way done) then I can finish the rewrite (sans the ugly libevent/pthread mix that it currently has) and upstream the entire thing.

I’ve not looking into the GDBus interface, just the old glib-dbus.

Any advice on how to handle that particular problem would be especially helpful.

Thanks,

-Philip