On 03/08/2016 05:09 PM, Pavel Reichl wrote:
Hello Petr,

I just run through the code and I have some code style suggestions, feel
free to disagree :-).

I will address some suggestions tomorrow. I am doing tests for sysdb_sudo_rules now. Thanks for comments.

Petr


On 03/08/2016 01:11 PM, Petr Cech wrote:
0001-SYSDB-Add-new-funtions-into-sysdb_sudo.patch
...
+
+errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
+                                struct sss_domain_info *domain,
+                                const char *sub_filter,
+                                const char **attrs,
+                                size_t *msgs_count,
+                                struct ldb_message ***msgs)

if count and msgs are output parameters then please add _ as a prefix.

+{
+    TALLOC_CTX *tmp_ctx;
+    struct ldb_dn *dn;
+    char *filter;
+    int ret;
errno_t might be better here.
+
+    tmp_ctx = talloc_new(NULL);
+    if (!tmp_ctx) {
+        return ENOMEM;
+    }

I think we prefere != NULL form - but this is not important.

+
+    dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb,
SYSDB_TMPL_CUSTOM_SUBTREE,
+                        SUDORULE_SUBDIR, domain->name);
+    if (!dn) {
+        DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
+        ret = ENOMEM;
+        goto done;
+    }
+
+    if (sub_filter != NULL) {
+        filter = talloc_asprintf(tmp_ctx, "(&%s%s)",
+                                 SUDO_ALL_FILTER, sub_filter);
+    } else {
+        filter = talloc_asprintf(tmp_ctx, "(&%s)", SUDO_ALL_FILTER);
+    }
+    if (!filter) {
+        DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
+        ret = ENOMEM;
+        goto done;
+    }
+
+    DEBUG(SSSDBG_TRACE_INTERNAL,
+          "Search sudo rules with filter: %s\n", filter);
+
+    ret = sysdb_search_entry(mem_ctx, domain->sysdb, dn,
+                             LDB_SCOPE_SUBTREE, filter, attrs,
+                             msgs_count, msgs);

You could use tmp_ctx context here, it would make code a bit longer
because you would have to steal but also a bit more mem leak resistant.
+
+    if (ret != EOK) {
+        if (ret == ENOENT) {
+            DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
+        }
+        else if (ret) {
+            DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
strerror(ret));
+        }
+    }

    You can save indentation.

    if (ret == ENOENT) {
    ...
    } else if (ret != EOK) {
    ...
    }

   Please use sss_strerror instead of strerror().


+
+done:
+    talloc_zfree(tmp_ctx);
+    return ret;
+}
+



0002-TOOL-Invalidation-of-sudo-rules-at-sss_cache.patch


 From e0143502fce82d353955352d8717202346fed6b6 Mon Sep 17 00:00:00 2001
From: Petr Cech<pc...@redhat.com>
Date: Tue, 23 Feb 2016 10:11:15 -0500
Subject: [PATCH 2/2] TOOL: Invalidation of sudo rules at sss_cache

This patch adds new functionality to sss_cach for invalidation of given
sudo rule or all sudo rules.

Resolves:
https://fedorahosted.org/sssd/ticket/2081
---
...
              }
@@ -577,6 +611,7 @@ errno_t init_context(int argc, const char *argv[],
struct cache_tool_ctx **tctx)
      char *service = NULL;
      char *map = NULL;
      char *ssh_host = NULL;
+    char *sudo_rule = NULL;
      char *domain = NULL;
      int debug = SSSDBG_DEFAULT;
      errno_t ret = EOK;
@@ -616,6 +651,12 @@ errno_t init_context(int argc, const char
*argv[], struct cache_tool_ctx **tctx)
          { "ssh-hosts", 'H', POPT_ARG_NONE, NULL, 'h',
              _("Invalidate all SSH hosts"), NULL },
  #endif /* BUILD_SSH */
+#ifdef BUILD_SUDO
+        { "sudo-rule", 'r', POPT_ARG_STRING, &sudo_rule, 0,
+            _("Invalidate particular sudo rule"), NULL },
+        { "sudo-rules", 'R', POPT_ARG_NONE, NULL, 'r',
+            _("Invalidate all cached sudo rules"), NULL },
+#endif /* BUILD_SUDO */
          { "domain", 'd', POPT_ARG_STRING, &domain, 0,
              _("Only invalidate entries from a particular domain"),
NULL },
          POPT_TABLEEND
@@ -650,8 +691,12 @@ errno_t init_context(int argc, const char
*argv[], struct cache_tool_ctx **tctx)
              case 'h':
                  idb |= INVALIDATE_SSH_HOSTS;
                  break;
+            case 'r':
+                idb |= INVALIDATE_SUDO_RULES;
+                break;
              case 'e':
                  idb = INVALIDATE_EVERYTHING;
+                idb |= INVALIDATE_SUDO_RULES;
                  break;
          }
      }
@@ -664,7 +709,7 @@ errno_t init_context(int argc, const char *argv[],
struct cache_tool_ctx **tctx)
      }

      if (idb == INVALIDATE_NONE && !user && !group &&
-        !netgroup && !service && !map && !ssh_host) {
+        !netgroup && !service && !map && !ssh_host && !sudo_rule) {
          BAD_POPT_PARAMS(pc,
                  _("Please select at least one object to invalidate\n"),
                  ret, fini);
@@ -729,18 +774,28 @@ errno_t init_context(int argc, const char
*argv[], struct cache_tool_ctx **tctx)
          ctx->update_ssh_host_filter = true;
      }

+    if (idb & INVALIDATE_SUDO_RULES) {
+        ctx->sudo_rule_filter = talloc_asprintf(ctx, "(%s=*)",
SYSDB_NAME);
+        ctx->update_sudo_rule_filter = false;
+    } else if (sudo_rule) {
+        ctx->sudo_rule_name = talloc_strdup(ctx, sudo_rule);
+        ctx->update_sudo_rule_filter = true;
+    }
+
      if (((idb & INVALIDATE_USERS) && !ctx->user_filter) ||
          ((idb & INVALIDATE_GROUPS) && !ctx->group_filter) ||
          ((idb & INVALIDATE_NETGROUPS) && !ctx->netgroup_filter) ||
          ((idb & INVALIDATE_SERVICES) && !ctx->service_filter) ||
          ((idb & INVALIDATE_AUTOFSMAPS) && !ctx->autofs_filter) ||
          ((idb & INVALIDATE_SSH_HOSTS) && !ctx->ssh_host_filter) ||
+        ((idb & INVALIDATE_SUDO_RULES) && !ctx->sudo_rule_filter) ||
           (user && !ctx->user_name) ||
           (group && !ctx->group_name) ||
           (netgroup && !ctx->netgroup_name) ||
           (service && !ctx->service_name) ||
           (map && !ctx->autofs_name) ||
-         (ssh_host && !ctx->ssh_host_name)) {
+         (ssh_host && !ctx->ssh_host_name) ||
+         (sudo_rule && !ctx->sudo_rule_name)) {

Can we refactor and move to a function?

          DEBUG(SSSDBG_CRIT_FAILURE, "Construction of filters failed\n");
          ret = ENOMEM;
          goto fini;
@@ -768,6 +823,10 @@ fini:
      free(group);
      free(netgroup);
      free(domain);
+    free(service);
+    free(map);
+    free(ssh_host);
+    free(sudo_rule);

Can we bundle this vars into a new struct and free it in a seperate
function?

      if (ret != EOK && ctx) {
          talloc_zfree(ctx);
      }
-- 2.5.0



_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org


--
Petr^4 Čech
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

Reply via email to