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

[Philip Prindeville]

> On Oct 5, 2016, at 9:43 AM, Jakub Hrozek <jhro...@redhat.com> wrote:
> 
> On Wed, Oct 05, 2016 at 07:42:23AM -0600, Philip Prindeville wrote:
>> On Oct 5, 2016, at 5:45 AM, Michal Židek <mzi...@redhat.com> 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-examples) 
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


___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

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

[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


___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

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

[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
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

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

[Philip Prindeville]

From: Philip Prindeville <phil...@fedoraproject.org>

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, 
>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:
 snp

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

[Philip Prindeville]

Is there a ding-libs website?

I wanted to subscribe to the mailing list and read the archives but 
fedorahosted.org only seems to point to the git repo… nothing about a ding-libs 
general website, etc.


> On Sep 29, 2016, at 4:50 AM, Michal Židek <mzi...@redhat.com> wrote:
> 
> Moving this to sssd-devel list. So that other
> developers can see the patch and review process.
> 
> I will start the review after today's meeting.
> 
> Michal
> 
> (Lukas, Stephen, I put you two to CC only because you
> were also original recipients of the mail, I will not
> CC you further)
> 
> On 09/28/2016 03:00 AM, Philip Prindeville wrote:
>> From: Philip Prindeville <phil...@fedoraproject.org>
>> 
>> Add c_str as a "const char *" to the hash_key_t.
>> 
>> ---
>>  dhash/dhash.c   | 35 ++-
>>  dhash/dhash.h   |  4 +++-
>>  dhash/examples/dhash_test.c |  3 +++
>>  3 files changed, 36 insertions(+), 6 deletions(-)
>> 
>> diff --git a/dhash/dhash.c b/dhash/dhash.c
>> index 45ee0cf..5f9f631 100644
>> --- a/dhash/dhash.c
>> +++ b/dhash/dhash.c
>> @@ -44,6 +44,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include "dhash.h"
>> 
>> @@ -70,6 +71,9 @@
>>  } \
>>  } while(0)
>> 
>> +#define discard_const(ptr) ((void *)((intptr_t)(ptr)))
>> +#define discard_const_p(type, ptr) ((type *)discard_const(ptr))
>> +
>>  
>> /*/
>>  /** Internal Type Definitions 
>> /
>>  
>> /*/
>> @@ -176,7 +180,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 +188,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 +221,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 +254,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;
>>  }
>> @@ -671,7 +683,9 @@ int hash_destroy(hash_table_t *table)
>>  q = p->next;
>>  hdelete_callback(table, HASH_TABLE_DESTROY, 
>> >entry);
>>  if (p->entry.key.type == HASH_KEY_STRING) {
>> -hfree(table, (char *)p->entry.key.str);
>> +hfree(table, p->entry.key.str);
>> +} else if (p->entry.key.type == 
>> HASH_KEY_CONST_STRING) {
>> +hfree(table, 
>> discard_const(p->entry.key.c_str));
>>  }
>>  hfree(table, (char *)p);
>>  p = q;
>> @@ -978,7 +992,16 @@ int hash_enter(hash_table_t *table, hash_key_t *key, 
>> hash_value_t *value)
>>  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;
>> +case HASH_KEY_CONST_STRING:
>> +