SYSDB: Wrap sysdb_store_group in a transaction and split it into smaller functions

sysdb_store_group can do several things -- add, rename or update a
group. It's important they are all done in a single transaction (even
though the caller should typically start a transaction of his own).

Also split the sysdb_store_group function into two, one that only stores
a new group and one that changes attributes of an existing group to keep
the flow easy and avoid two labels in a single function.

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

1 files changed, 107 insertions, 42 deletions

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 3c9a85395..79d7eef9e 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -2024,6 +2024,20 @@ fail:
 
 /* this function does not check that all user members are actually present */
 
+static errno_t sysdb_store_new_group(struct sss_domain_info *domain,
+                                     const char *name,
+                                     gid_t gid,
+                                     struct sysdb_attrs *attrs,
+                                     uint64_t cache_timeout,
+                                     time_t now);
+
+static errno_t sysdb_store_group_attrs(struct sss_domain_info *domain,
+                                       const char *name,
+                                       gid_t gid,
+                                       struct sysdb_attrs *attrs,
+                                       uint64_t cache_timeout,
+                                       time_t now);
+
 int sysdb_store_group(struct sss_domain_info *domain,
                       const char *name,
                       gid_t gid,
@@ -2037,12 +2051,21 @@ int sysdb_store_group(struct sss_domain_info *domain,
     struct ldb_message *msg;
     bool new_group = false;
     int ret;
+    errno_t sret = EOK;
+    bool in_transaction = false;
 
     tmp_ctx = talloc_new(NULL);
     if (!tmp_ctx) {
         return ENOMEM;
     }
 
+    ret = sysdb_transaction_start(domain->sysdb);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Failed to start transaction\n");
+        goto done;
+    }
+    in_transaction = true;
+
     ret = sysdb_search_group_by_name(tmp_ctx, domain, name, src_attrs, &msg);
     if (ret && ret != ENOENT) {
         DEBUG(SSSDBG_MINOR_FAILURE,
@@ -2072,54 +2095,102 @@ int sysdb_store_group(struct sss_domain_info *domain,
      * group needs any update */
 
     if (new_group) {
-        /* group doesn't exist, turn into adding a group */
-        ret = sysdb_add_group(domain, name, gid, attrs, cache_timeout,
-                              now);
-        if (ret == EEXIST) {
-            /* This may be a group rename. If there is a group with the
-             * same GID, remove it and try to add the basic group again
-             */
-            DEBUG(SSSDBG_TRACE_LIBS, "sysdb_add_group failed: [EEXIST].\n");
-            ret = sysdb_delete_group(domain, NULL, gid);
-            if (ret == ENOENT) {
-                /* Not found by GID, return the original EEXIST,
-                 * this may be a conflict in MPG domain or something
-                 * else */
-                DEBUG(SSSDBG_TRACE_LIBS,
-                      "sysdb_delete_group failed (while renaming group). Not "
-                      "found by gid: [%"SPRIgid"].\n", gid);
-                return EEXIST;
-            } else if (ret != EOK) {
-                DEBUG(SSSDBG_TRACE_LIBS, "sysdb_add_group failed.\n");
-                goto done;
-            }
+        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);
+    }
+
+    sret = sysdb_transaction_commit(domain->sysdb);
+    if (sret != EOK) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Failed to commit transaction\n");
+        ret = EIO;
+        goto done;
+    }
+    in_transaction = false;
+
+done:
+    if (in_transaction) {
+        sret = sysdb_transaction_cancel(domain->sysdb);
+        if (sret != EOK) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "Could not cancel transaction\n");
+        }
+    }
+
+    if (ret) {
+        DEBUG(SSSDBG_TRACE_FUNC, "Error: %d (%s)\n", ret, strerror(ret));
+    }
+    talloc_zfree(tmp_ctx);
+    return ret;
+}
+
+
+static errno_t sysdb_store_new_group(struct sss_domain_info *domain,
+                                     const char *name,
+                                     gid_t gid,
+                                     struct sysdb_attrs *attrs,
+                                     uint64_t cache_timeout,
+                                     time_t now)
+{
+    errno_t ret;
+
+    /* group doesn't exist, turn into adding a group */
+    ret = sysdb_add_group(domain, name, gid, attrs, cache_timeout, now);
+    if (ret == EEXIST) {
+        /* This may be a group rename. If there is a group with the
+         * same GID, remove it and try to add the basic group again
+         */
+        DEBUG(SSSDBG_TRACE_LIBS, "sysdb_add_group failed: [EEXIST].\n");
+        ret = sysdb_delete_group(domain, NULL, gid);
+        if (ret == ENOENT) {
+            /* Not found by GID, return the original EEXIST,
+             * this may be a conflict in MPG domain or something
+             * else */
+            DEBUG(SSSDBG_TRACE_LIBS,
+                  "sysdb_delete_group failed (while renaming group). Not "
+                  "found by gid: [%"SPRIgid"].\n", gid);
+            return EEXIST;
+        } else if (ret != EOK) {
+            DEBUG(SSSDBG_TRACE_LIBS, "sysdb_add_group failed.\n");
+            return ret;
+        }
+
+        DEBUG(SSSDBG_MINOR_FAILURE,
+                "A group with the same GID [%"SPRIgid"] was removed from "
+                "the cache\n", gid);
+        ret = sysdb_add_group(domain, name, gid, attrs, cache_timeout, now);
+        if (ret) {
             DEBUG(SSSDBG_MINOR_FAILURE,
-                  "A group with the same GID [%"SPRIgid"] was removed from "
-                  "the cache\n", gid);
-            ret = sysdb_add_group(domain, name, gid, attrs, cache_timeout,
-                                  now);
-            if (ret) {
-                DEBUG(SSSDBG_MINOR_FAILURE,
-                      "sysdb_add_group failed (while renaming group) for: "
-                      "%s [%"SPRIgid"].\n", name, gid);
-            }
+                    "sysdb_add_group failed (while renaming group) for: "
+                    "%s [%"SPRIgid"].\n", name, gid);
+            return ret;
         }
-        goto done;
     }
 
+    return EOK;
+}
+
+static errno_t sysdb_store_group_attrs(struct sss_domain_info *domain,
+                                       const char *name,
+                                       gid_t gid,
+                                       struct sysdb_attrs *attrs,
+                                       uint64_t cache_timeout,
+                                       time_t now)
+{
+    errno_t ret;
+
     /* the group exists, let's just replace attributes when set */
     if (gid) {
         ret = sysdb_attrs_add_uint32(attrs, SYSDB_GIDNUM, gid);
         if (ret) {
             DEBUG(SSSDBG_TRACE_LIBS, "Failed to add GID.\n");
-            goto done;
+            return ret;
         }
     }
 
     ret = sysdb_attrs_add_time_t(attrs, SYSDB_LAST_UPDATE, now);
     if (ret) {
         DEBUG(SSSDBG_TRACE_LIBS, "Failed to add sysdb-last-update.\n");
-        goto done;
+        return ret;
     }
 
     ret = sysdb_attrs_add_time_t(attrs, SYSDB_CACHE_EXPIRE,
@@ -2127,24 +2198,18 @@ int sysdb_store_group(struct sss_domain_info *domain,
                                   (now + cache_timeout) : 0));
     if (ret) {
         DEBUG(SSSDBG_TRACE_LIBS, "Failed to add sysdb-cache-expire.\n");
-        goto done;
+        return ret;
     }
 
     ret = sysdb_set_group_attr(domain, name, attrs, SYSDB_MOD_REP);
     if (ret) {
         DEBUG(SSSDBG_TRACE_LIBS, "sysdb_set_group_attr failed.\n");
-        goto done;
+        return ret;
     }
 
-done:
-    if (ret) {
-        DEBUG(SSSDBG_TRACE_FUNC, "Error: %d (%s)\n", ret, strerror(ret));
-    }
-    talloc_zfree(tmp_ctx);
-    return ret;
+    return EOK;
 }
 
-
 /* =Add-User-to-Group(Native/Legacy)====================================== */
 static int
 sysdb_group_membership_mod(struct sss_domain_info *domain,