SYSDB: Check if group attributes differ before saving a group

Adds a new function sysdb_entry_attrs_diff() used in group saving code.
This function is used to check if the result of updating a group would
result in actually changing the sysdb entry -- often, we would try to
dump the same data to the cache during update. If that's the case, the
update code now only updates the timestamp cache, avoiding costly
writes.

Reviewed-by: Sumit Bose <sbose@redhat.com>

3 files changed, 139 insertions, 7 deletions

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 1224e96ad..cb35d1c65 100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1791,3 +1791,118 @@ bool sysdb_msg_attrs_modts_differs(struct ldb_message *old_entry,
 
     return true;
 }
+
+static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
+                                     struct ldb_message *mod_msg)
+{
+    struct ldb_message_element *mod_msg_el;
+    struct ldb_message_element *db_msg_el;
+    int el_differs;
+
+    for (unsigned i = 0; i < mod_msg->num_elements; i++) {
+        mod_msg_el = &mod_msg->elements[i];
+
+        switch (mod_msg_el->flags) {
+        case 0:
+        /* Unspecified flags are internally converted to SYSDB_MOD_REP in
+         * sysdb_set_group_attr, do the same here
+         */
+        case SYSDB_MOD_ADD:
+        case SYSDB_MOD_REP:
+            db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name);
+            if (db_msg_el == NULL) {
+                /* The attribute to be added does not exist in the target
+                 * message, this is a modification. Special-case adding
+                 * empty elements which also do not exist in the target
+                 * message. This is how sysdb callers ensure a particular
+                 * element is not present in the database.
+                 */
+                if (mod_msg_el->num_values > 0) {
+                    /* We can ignore additions of timestamp attributes */
+                    return true;
+                }
+                break;
+            }
+
+            el_differs = ldb_msg_element_compare(db_msg_el, mod_msg_el);
+            if (el_differs) {
+                /* We are replacing or extending element, there is a difference. If
+                 * some values already exist and ldb_add is not permissive,
+                 * ldb will throw an error, but that's not our job to check..
+                 */
+                if (is_ts_cache_attr(mod_msg_el->name) == false) {
+                    /* We can ignore changes to timestamp attributes */
+                    return true;
+                }
+            }
+            break;
+        case SYSDB_MOD_DEL:
+            db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name);
+            if (db_msg_el != NULL) {
+                /* We are deleting a valid element, there is a difference */
+                return true;
+            }
+            break;
+        }
+    }
+
+    return false;
+}
+
+bool sysdb_entry_attrs_diff(struct sysdb_ctx *sysdb,
+                            struct ldb_dn *entry_dn,
+                            struct sysdb_attrs *attrs,
+                            int mod_op)
+{
+    struct ldb_message *new_entry_msg = NULL;
+    TALLOC_CTX *tmp_ctx;
+    bool differs = true;
+    int lret;
+    errno_t ret;
+    struct ldb_result *res;
+    const char *attrnames[attrs->num+1];
+
+    if (sysdb->ldb_ts == NULL) {
+        return true;
+    }
+
+    if (is_ts_ldb_dn(entry_dn) == false) {
+        return true;
+    }
+
+    tmp_ctx = talloc_new(NULL);
+    if (tmp_ctx == NULL) {
+        goto done;
+    }
+
+    new_entry_msg = sysdb_attrs2msg(tmp_ctx, entry_dn,
+                                    attrs, mod_op);
+    if (new_entry_msg == NULL) {
+        goto done;
+    }
+
+    for (int i = 0; i < attrs->num; i++) {
+        attrnames[i] = attrs->a[i].name;
+    }
+    attrnames[attrs->num] = NULL;
+
+    lret = ldb_search(sysdb->ldb, tmp_ctx, &res, entry_dn, LDB_SCOPE_BASE,
+                      attrnames, NULL);
+    if (lret != LDB_SUCCESS) {
+        ret = sysdb_error_to_errno(lret);
+        DEBUG(SSSDBG_MINOR_FAILURE, "Cannot search sysdb: %d\n", ret);
+        goto done;
+    }
+
+    if (res->count == 0) {
+        return true;
+    } else if (res->count != 1) {
+        ret = EIO;
+        goto done;
+    }
+
+    differs = sysdb_ldb_msg_difference(res->msgs[0], new_entry_msg);
+done:
+    talloc_free(tmp_ctx);
+    return differs;
+}
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 4ea0d0ad7..eca6ca1d9 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -1172,10 +1172,15 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
                          struct sysdb_attrs *attrs,
                          int mod_op)
 {
+    bool sysdb_write = true;
     errno_t ret = EOK;
     errno_t tret = EOK;
 
-    ret = sysdb_set_cache_entry_attr(sysdb->ldb, entry_dn, attrs, mod_op);
+    sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs, mod_op);
+    if (sysdb_write == true) {
+        ret = sysdb_set_cache_entry_attr(sysdb->ldb, entry_dn, attrs, mod_op);
+    }
+
     if (ret == EOK) {
         tret = sysdb_set_ts_entry_attr(sysdb, entry_dn, attrs, mod_op);
         if (tret != EOK) {
@@ -2563,6 +2568,7 @@ static errno_t sysdb_store_new_group(struct sss_domain_info *domain,
 static errno_t sysdb_store_group_attrs(struct sss_domain_info *domain,
                                        const char *name,
                                        gid_t gid,
+                                       struct ldb_message *cached_group,
                                        struct sysdb_attrs *attrs,
                                        uint64_t cache_timeout,
                                        time_t now);
@@ -2575,7 +2581,7 @@ int sysdb_store_group(struct sss_domain_info *domain,
                       time_t now)
 {
     TALLOC_CTX *tmp_ctx;
-    static const char *src_attrs[] = { SYSDB_NAME, SYSDB_GIDNUM, NULL };
+    static const char *src_attrs[] = { "*", NULL };
     struct ldb_message *msg;
     bool new_group = false;
     int ret;
@@ -2628,13 +2634,12 @@ int sysdb_store_group(struct sss_domain_info *domain,
         now = time(NULL);
     }
 
-    /* FIXME: use the remote modification timestamp to know if the
-     * group needs any update */
-
     if (new_group) {
-        ret = sysdb_store_new_group(domain, name, gid, attrs, cache_timeout, now);
+        ret = sysdb_store_new_group(domain, name, gid, attrs,
+                                    cache_timeout, now);
     } else {
-        ret = sysdb_store_group_attrs(domain, name, gid, attrs, cache_timeout, now);
+        ret = sysdb_store_group_attrs(domain, name, gid, msg, attrs,
+                                      cache_timeout, now);
     }
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE, "Cache update failed: %d\n", ret);
@@ -2713,6 +2718,7 @@ static errno_t sysdb_store_new_group(struct sss_domain_info *domain,
 static errno_t sysdb_store_group_attrs(struct sss_domain_info *domain,
                                        const char *name,
                                        gid_t gid,
+                                       struct ldb_message *cached_group,
                                        struct sysdb_attrs *attrs,
                                        uint64_t cache_timeout,
                                        time_t now)
diff --git a/src/db/sysdb_private.h b/src/db/sysdb_private.h
index 9d03d59f0..6f6a19a74 100644
--- a/src/db/sysdb_private.h
+++ b/src/db/sysdb_private.h
@@ -264,4 +264,15 @@ struct ldb_message *sysdb_attrs2msg(TALLOC_CTX *mem_ctx,
                                     struct sysdb_attrs *attrs,
                                     int mod_op);
 
+/* Compares the attributes between the existing attributes of entry_dn and
+ * the new_entry attributes that are about to be set. If the set would
+ * not yield into any differences (and therefore a write to the cache is
+ * not necessary), the function returns false (no diff), otherwise
+ * the function returns true (a difference exists).
+ */
+bool sysdb_entry_attrs_diff(struct sysdb_ctx *sysdb,
+                            struct ldb_dn *entry_dn,
+                            struct sysdb_attrs *attrs,
+                            int mod_op);
+
 #endif /* __INT_SYS_DB_H__ */