[SSSD] [PATCH] LDAP: Compare lists of DNs when saving autofs entries (sssd-1-9)

Jakub Hrozek jhrozek at redhat.com
Sat Jan 19 14:28:00 UTC 2013


On Fri, Jan 18, 2013 at 01:56:02PM +0100, Pavel Březina wrote:
> On 01/18/2013 11:25 AM, Jakub Hrozek wrote:
> >On Fri, Jan 18, 2013 at 10:47:34AM +0100, Pavel Březina wrote:
> >>On 01/16/2013 03:20 PM, Jakub Hrozek wrote:
> >>>This patch applies on top of sssd-1-9
> >>>
> >>>https://fedorahosted.org/sssd/ticket/1758
> >>>
> >>>The autofs entries do not have the key as an unique identifier, but
> >>>rather the full (key, value) tuple as some keys have a special meaning,
> >>>such as the direct mount key (/-) and may be present in a single map
> >>>multiple times.
> >>>
> >>>Comparing the full DN that contains both the key and the value will
> >>>allow for working updates if either key or value changes.
> >>
> >>Hi,
> >>the patch does not apply to current master.
> >
> >See original mail:
> >"This patch applies on top of sssd-1-9"
> >
> >Originally I was trying to meet a deadline so I went with 1.9 first. But
> >you're right we need to fix the same problem in master, too. I will
> >prepare a version for master.
> 
> Ah, my bad.
> 
> Nack.
> 
> get_autofs_entry_attr(): we should also check if el->num_values > 1 and
> error out if it happens.
> 

Well, I think it's better to try to carry, but the new patch simply
errors out if num_values != 1.

> del_autofs_entries(): please, remove the FIXME :-) If you want to wrap
> it in sysdb function, it can be done in a minute. If you don't I don't
> mind in this case.
> 

OK, I moved the code into sysdb where it logically belongs.

> sdap_autofs_setautomntent_save(): you don't use same_entries anywhere


Thanks, I was using it during debugging to see if the diff returns the
expected results and forgot to remove it.

Thank you for the review, a new patch is attached. I also sent a patch
that applies on master with these comments factored in.
-------------- next part --------------
>From d351609908719d67a97ed5d55cee8233eae604af Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 13 Jan 2013 12:06:40 +0100
Subject: [PATCH] LDAP: Compare lists of DNs when saving autofs entries

https://fedorahosted.org/sssd/ticket/1758

The autofs entries do not have the key as an unique identifier, but
rather the full (key, value) tuple as some keys have a special meaning,
such as the direct mount key (/-) and may be present in a single map
multiple times.

Comparing the full DN that contains both the key and the value will
allow for working updates if either key or value changes.
---
 src/db/sysdb_autofs.c                  |  25 ++-
 src/db/sysdb_autofs.h                  |  11 +-
 src/providers/ldap/sdap_async_autofs.c | 298 +++++++++++++++++----------------
 3 files changed, 185 insertions(+), 149 deletions(-)

diff --git a/src/db/sysdb_autofs.c b/src/db/sysdb_autofs.c
index 57380cfdcde2bc43befcaee94c7d9f540dd88b97..f5aad259a2757300e77ee4326a175610c3a11bd8 100644
--- a/src/db/sysdb_autofs.c
+++ b/src/db/sysdb_autofs.c
@@ -77,6 +77,25 @@ done:
     return dn;
 }
 
+char *
+sysdb_autofsentry_strdn(TALLOC_CTX *mem_ctx,
+                        struct sysdb_ctx *sysdb,
+                        const char *map_name,
+                        const char *entry_name,
+                        const char *entry_value)
+{
+    struct ldb_dn *dn;
+    char *strdn;
+
+    dn = sysdb_autofsentry_dn(mem_ctx, sysdb,
+                              map_name, entry_name, entry_value);
+    if (!dn) return NULL;
+
+    strdn = talloc_strdup(mem_ctx, ldb_dn_get_linearized(dn));
+    talloc_free(dn);
+    return strdn;
+}
+
 errno_t
 sysdb_save_autofsmap(struct sysdb_ctx *sysdb_ctx,
                      const char *name,
@@ -314,14 +333,12 @@ done:
 
 errno_t
 sysdb_del_autofsentry(struct sysdb_ctx *sysdb_ctx,
-                      const char *map,
-                      const char *key,
-                      const char *value)
+                      const char *entry_dn)
 {
     struct ldb_dn *dn;
     errno_t ret;
 
-    dn = sysdb_autofsentry_dn(sysdb_ctx, sysdb_ctx, map, key, value);
+    dn = ldb_dn_new(NULL, sysdb_ctx_get_ldb(sysdb_ctx), entry_dn);
     if (!dn) {
         return ENOMEM;
     }
diff --git a/src/db/sysdb_autofs.h b/src/db/sysdb_autofs.h
index 8c8d0f560ff5aa40cb8a7e5513dc36d1c09cd896..0e54f9dc12ca6a9ebc9429c55a02ee4b5228c226 100644
--- a/src/db/sysdb_autofs.h
+++ b/src/db/sysdb_autofs.h
@@ -61,9 +61,7 @@ sysdb_save_autofsentry(struct sysdb_ctx *sysdb_ctx,
                        struct sysdb_attrs *attrs);
 errno_t
 sysdb_del_autofsentry(struct sysdb_ctx *sysdb_ctx,
-                      const char *map,
-                      const char *key,
-                      const char *value);
+                      const char *entry_dn);
 
 errno_t
 sysdb_autofs_entries_by_map(TALLOC_CTX *mem_ctx,
@@ -81,4 +79,11 @@ sysdb_set_autofsmap_attr(struct sysdb_ctx *sysdb,
 errno_t
 sysdb_invalidate_autofs_maps(struct sysdb_ctx *sysdb);
 
+char *
+sysdb_autofsentry_strdn(TALLOC_CTX *mem_ctx,
+                        struct sysdb_ctx *sysdb,
+                        const char *map_name,
+                        const char *entry_name,
+                        const char *entry_value);
+
 #endif /* _SYSDB_AUTOFS_H_ */
diff --git a/src/providers/ldap/sdap_async_autofs.c b/src/providers/ldap/sdap_async_autofs.c
index fdffd1d5a270035831be7ac86139afc8eb57a2c5..dde2effaf39b043d5cdcbe3afc05ea63f469ea12 100644
--- a/src/providers/ldap/sdap_async_autofs.c
+++ b/src/providers/ldap/sdap_async_autofs.c
@@ -50,149 +50,136 @@ get_autofs_map_name(struct sysdb_attrs *map, struct sdap_options *opts)
 }
 
 static const char *
+get_autofs_entry_attr(struct sysdb_attrs *entry, struct sdap_options *opts,
+                      enum sdap_autofs_entry_attrs attr)
+{
+    errno_t ret;
+    struct ldb_message_element *el;
+
+    ret = sysdb_attrs_get_el(entry,
+            opts->autofs_entry_map[attr].sys_name,
+            &el);
+    if (ret) return NULL;
+    if (el->num_values != 1) {
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("Expected one entry got %d\n", el->num_values));
+        return NULL;
+    }
+
+    return (const char *)el->values[0].data;
+}
+
+static const char *
 get_autofs_entry_key(struct sysdb_attrs *entry, struct sdap_options *opts)
 {
-    errno_t ret;
-    struct ldb_message_element *el;
+    return get_autofs_entry_attr(entry, opts, SDAP_AT_AUTOFS_ENTRY_KEY);
+}
 
-    ret = sysdb_attrs_get_el(entry,
-            opts->autofs_entry_map[SDAP_AT_AUTOFS_ENTRY_KEY].sys_name,
-            &el);
-    if (ret) return NULL;
-    if (el->num_values == 0) return NULL;
-
-    return (const char *)el->values[0].data;
+static const char *
+get_autofs_entry_value(struct sysdb_attrs *entry, struct sdap_options *opts)
+{
+    return get_autofs_entry_attr(entry, opts, SDAP_AT_AUTOFS_ENTRY_VALUE);
 }
 
 static errno_t
-mod_autofs_entry(struct sysdb_ctx *sysdb,
+add_autofs_entry(struct sysdb_ctx *sysdb,
                  const char *map,
                  struct sdap_options *opts,
-                 struct sysdb_attrs *entry,
-                 enum autofs_map_op mod_op)
+                 struct sysdb_attrs *entry)
 {
     const char *key;
     const char *value;
-    struct ldb_message_element *el;
-    errno_t ret;
 
     key = get_autofs_entry_key(entry, opts);
-    if (!key) return EINVAL;
-
-    ret = sysdb_attrs_get_el(entry,
-            opts->autofs_entry_map[SDAP_AT_AUTOFS_ENTRY_VALUE].sys_name,
-            &el);
-    if (ret) return ret;
-    if (el->num_values == 0) return EINVAL;
-    else value = (const char *)el->values[0].data;
-
-    switch (mod_op) {
-    case AUTOFS_MAP_OP_ADD:
-        ret = sysdb_save_autofsentry(sysdb, map, key, value, NULL);
-        break;
-    case AUTOFS_MAP_OP_DEL:
-        ret = sysdb_del_autofsentry(sysdb, map, key, value);
-        break;
-    }
-
-    return ret;
-}
-
-static errno_t
-mod_autofs_entries(struct sysdb_ctx *sysdb,
-                   struct sdap_options *opts,
-                   const char *map,
-                   char **mod_entries,
-                   struct sysdb_attrs **entries,
-                   size_t num_entries,
-                   enum autofs_map_op mod_op)
-
-{
-    errno_t ret, tret;
-    const char *key;
-    bool in_transaction = false;
-    int i, j;
-
-    ret = sysdb_transaction_start(sysdb);
-    if (ret != EOK) {
-        DEBUG(SSSDBG_CRIT_FAILURE,
-             ("Cannot start sysdb transaction [%d]: %s\n",
-              ret, strerror(ret)));
-        goto done;
-    }
-    in_transaction = true;
-
-    /* Loop through entry names.. */
-    for (i=0; mod_entries[i]; i++) {
-        for (j=0; j < num_entries; j++) {
-            /* get a pointer to sysdb_attrs of an entry that is not
-             * cached, skip names that are not in **entries
-             */
-            key = get_autofs_entry_key(entries[j], opts);
-            if (!key) {
-                DEBUG(SSSDBG_MINOR_FAILURE,
-                      ("An entry without name? Skipping\n"));
-                return EINVAL;
-            }
-
-            if (strcmp(mod_entries[i], key)) {
-                continue;
-            }
-
-            ret = mod_autofs_entry(sysdb, map, opts, entries[j], mod_op);
-            if (ret != EOK) {
-                DEBUG(SSSDBG_OP_FAILURE,
-                    ("Cannot modify autofs entry [%d]: %s. Ignoring.\n",
-                    ret, strerror(ret)));
-                continue;
-            }
-        }
+    if (!key) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Could not get autofs entry key\n"));
+        return EINVAL;
     }
 
-    ret = sysdb_transaction_commit(sysdb);
-    if (ret != EOK) {
-        DEBUG(SSSDBG_CRIT_FAILURE,
-             ("Cannot commit sysdb transaction [%d]: %s\n",
-              ret, strerror(ret)));
-        goto done;
+    value = get_autofs_entry_value(entry, opts);
+    if (!value) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Could not get autofs entry value\n"));
+        return EINVAL;
     }
-    in_transaction = false;
 
-    ret = EOK;
-done:
-    if (in_transaction) {
-        tret = sysdb_transaction_cancel(sysdb);
-        if (tret != EOK) {
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                ("Cannot cancel sysdb transaction [%d]: %s\n",
-                ret, strerror(ret)));
-        }
-    }
-    return ret;
+    return sysdb_save_autofsentry(sysdb, map, key, value, NULL);
 }
 
 static errno_t
 save_autofs_entries(struct sysdb_ctx *sysdb,
                     struct sdap_options *opts,
                     const char *map,
-                    char **add_entries,
-                    struct sysdb_attrs **entries,
-                    size_t num_entries)
+                    char **add_dn_list,
+                    hash_table_t *entry_hash)
 {
-    return mod_autofs_entries(sysdb, opts, map, add_entries,
-                              entries, num_entries, AUTOFS_MAP_OP_ADD);
+    hash_key_t key;
+    hash_value_t value;
+    size_t i;
+    int hret;
+    errno_t ret;
+    struct sysdb_attrs *entry;
+
+    if (!add_dn_list) {
+        return EOK;
+    }
+
+    for (i=0; add_dn_list[i]; i++) {
+        key.type = HASH_KEY_STRING;
+        key.str = (char *) add_dn_list[i];
+
+        hret = hash_lookup(entry_hash, &key, &value);
+        if (hret != HASH_SUCCESS) {
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  ("Cannot retrieve entry [%s] from hash\n", add_dn_list[i]));
+            continue;
+        }
+
+        entry = talloc_get_type(value.ptr, struct sysdb_attrs);
+        if (!entry) {
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  ("Cannot retrieve entry [%s] from ptr\n", add_dn_list[i]));
+            continue;
+        }
+
+        DEBUG(SSSDBG_TRACE_FUNC,
+              ("Saving autofs entry [%s]\n", add_dn_list[i]));
+        ret = add_autofs_entry(sysdb, map, opts, entry);
+        if (ret) {
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  ("Cannot save entry [%s] to cache\n", add_dn_list[i]));
+            continue;
+        }
+
+        DEBUG(SSSDBG_TRACE_FUNC, ("Saved entry [%s]\n", add_dn_list[i]));
+    }
+
+    DEBUG(SSSDBG_TRACE_INTERNAL, ("All entries saved\n"));
+    return EOK;
 }
 
 static errno_t
 del_autofs_entries(struct sysdb_ctx *sysdb,
                    struct sdap_options *opts,
                    const char *map,
-                   char **add_entries,
-                   struct sysdb_attrs **entries,
-                   size_t num_entries)
+                   char **del_dn_list)
 {
-    return mod_autofs_entries(sysdb, opts, map, add_entries,
-                              entries, num_entries, AUTOFS_MAP_OP_DEL);
+    size_t i;
+    errno_t ret;
+
+    for (i=0; del_dn_list[i]; i++) {
+        DEBUG(SSSDBG_TRACE_FUNC,
+              ("Removing autofs entry [%s]\n", del_dn_list[i]));
+
+        ret = sysdb_del_autofsentry(sysdb, del_dn_list[i]);
+        if (ret) {
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  ("Cannot delete entry %s\n", del_dn_list[i]));
+            continue;
+        }
+    }
+
+    DEBUG(SSSDBG_TRACE_INTERNAL, ("All entries removed\n"));
+    return EOK;
 }
 
 static errno_t
@@ -794,18 +781,25 @@ sdap_autofs_setautomntent_save(struct tevent_req *req)
     bool in_transaction = false;
     TALLOC_CTX *tmp_ctx;
     struct ldb_message **entries = NULL;
-    struct sysdb_attrs **entries_attrs;
     size_t count;
+    const char *key;
     const char *val;
     char **sysdb_entrylist;
     char **ldap_entrylist;
     char **add_entries;
     char **del_entries;
-    size_t i;
+    size_t i, j;
+
+    hash_table_t *entry_hash;
+    hash_key_t hkey;
+    hash_value_t value;
+    int hret;
 
     tmp_ctx = talloc_new(NULL);
     if (!tmp_ctx) return ENOMEM;
 
+    DEBUG(SSSDBG_TRACE_LIBS,
+          ("Got %d map entries from LDAP\n", state->entries_count));
     if (state->entries_count == 0) {
         /* No entries for this map in LDAP.
          * We need to ensure that there are no entries
@@ -813,17 +807,50 @@ sdap_autofs_setautomntent_save(struct tevent_req *req)
          */
         ldap_entrylist = NULL;
     } else {
-        ret = sysdb_attrs_to_list(
-                tmp_ctx, state->entries,
-                state->entries_count,
-                state->opts->autofs_entry_map[SDAP_AT_AUTOFS_ENTRY_KEY].sys_name,
-                &ldap_entrylist);
-        if (ret != EOK) {
-            DEBUG(SSSDBG_OP_FAILURE,
-                  ("sysdb_attrs_primary_name_list failed [%d]: %s\n",
-                   ret, strerror(ret)));
+        ldap_entrylist = talloc_array(tmp_ctx, char *,
+                                      state->entries_count+1);
+        if (!ldap_entrylist) {
+            ret = ENOMEM;
             goto done;
         }
+
+        ret = sss_hash_create(state, 32, &entry_hash);
+        if (ret) {
+            goto done;
+        }
+
+        /* Get a list of the map members by DN */
+        for (i=0, j=0; i < state->entries_count; i++) {
+            key = get_autofs_entry_key(state->entries[i], state->opts);
+            val = get_autofs_entry_value(state->entries[i], state->opts);
+            if (!key || !val) {
+                DEBUG(SSSDBG_MINOR_FAILURE, ("Malformed entry, skipping\n"));
+                continue;
+            }
+
+            ldap_entrylist[j] = sysdb_autofsentry_strdn(ldap_entrylist,
+                                                        state->sysdb,
+                                                        state->mapname,
+                                                        key, val);
+            if (!ldap_entrylist[j]) {
+                ret = ENOMEM;
+                goto done;
+            }
+
+            hkey.type = HASH_KEY_STRING;
+            hkey.str = ldap_entrylist[j];
+            value.type = HASH_VALUE_PTR;
+            value.ptr = state->entries[i];
+
+            hret = hash_enter(entry_hash, &hkey, &value);
+            if (hret != HASH_SUCCESS) {
+                ret = EIO;
+                goto done;
+            }
+
+            j++;
+        }
+        ldap_entrylist[state->entries_count] = NULL;
     }
 
     ret = sysdb_autofs_entries_by_map(tmp_ctx, state->sysdb, state->mapname,
@@ -835,6 +862,7 @@ sdap_autofs_setautomntent_save(struct tevent_req *req)
         goto done;
     }
 
+    DEBUG(SSSDBG_TRACE_LIBS, ("Got %d map entries from sysdb\n", count));
     if (count == 0) {
         /* No map members for this map in sysdb currently */
         sysdb_entrylist = NULL;
@@ -845,16 +873,10 @@ sdap_autofs_setautomntent_save(struct tevent_req *req)
             goto done;
         }
 
-        /* Get a list of the map members by name only */
+        /* Get a list of the map members by DN */
         for (i=0; i < count; i++) {
-            val = ldb_msg_find_attr_as_string(entries[i],
-                                              SYSDB_AUTOFS_ENTRY_KEY, NULL);
-            if (!val) {
-                DEBUG(SSSDBG_MINOR_FAILURE, ("An entry with no value?\n"));
-                continue;
-            }
-
-            sysdb_entrylist[i] = talloc_strdup(sysdb_entrylist, val);
+            sysdb_entrylist[i] = talloc_strdup(sysdb_entrylist,
+                                        ldb_dn_get_linearized(entries[i]->dn));
             if (!sysdb_entrylist[i]) {
                 ret = ENOMEM;
                 goto done;
@@ -892,7 +914,7 @@ sdap_autofs_setautomntent_save(struct tevent_req *req)
     if (add_entries && add_entries[0]) {
         ret = save_autofs_entries(state->sysdb, state->opts,
                                   state->mapname, add_entries,
-                                  state->entries, state->entries_count);
+                                  entry_hash);
         if (ret != EOK) {
             DEBUG(SSSDBG_OP_FAILURE,
                   ("Cannot save autofs entries [%d]: %s\n",
@@ -903,16 +925,8 @@ sdap_autofs_setautomntent_save(struct tevent_req *req)
 
     /* Delete entries that don't exist anymore */
     if (del_entries && del_entries[0]) {
-        ret = sysdb_msg2attrs(tmp_ctx, count, entries, &entries_attrs);
-        if (ret != EOK) {
-            DEBUG(SSSDBG_OP_FAILURE,
-                  ("sysdb_msg2attrs failed: [%d]: %s\n", ret, strerror(ret)));
-            goto done;
-        }
-
         ret = del_autofs_entries(state->sysdb, state->opts,
-                                 state->mapname, del_entries,
-                                 entries_attrs, count);
+                                 state->mapname, del_entries);
         if (ret != EOK) {
             DEBUG(SSSDBG_OP_FAILURE,
                   ("Cannot delete autofs entries [%d]: %s\n",
-- 
1.8.0.2



More information about the sssd-devel mailing list