On Wed, Apr 13, 2011 at 04:09:32PM +0200, Jan Zelený wrote:
From: Jan Zeleny <jzeleny(a)redhat.com>
Date: Mon, 11 Apr 2011 09:46:30 -0400
Subject: [PATCH 2/3] Some minor fixes and changes in sysdb_ops
---
src/db/sysdb_ops.c | 53 +++++++++++++++++++++++++++++++++++----------------
1 files changed, 36 insertions(+), 17 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 7eb4b48c959c8d40d19b1179cbdbadbb0c4448f1..e74bcfa6ad5122fde32742b095bf09e9646d1026
100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -531,6 +531,10 @@ int sysdb_set_netgroup_attr(struct sysdb_ctx *ctx,
return ENOMEM;
}
+ if (domain == NULL) {
+ domain = ctx->domain;
+ }
+
dn = sysdb_netgroup_dn(ctx, tmp_ctx, domain->name, name);
if (!dn) {
ret = ENOMEM;
I personally don't quite like this idea. I think this makes the
API ambiguous. But I would like to hear other opinions.
Is there any reason why we should use another domain than the one in the
sysdb context?
I suspect that the separation of sysdb_ctx* and domain comes from when there
was only one cache file for all domains.
From: Jan Zeleny <jzeleny(a)redhat.com>
Date: Wed, 13 Apr 2011 08:16:37 -0400
Subject: [PATCH 3/3] Cache cleaning tool
I will fully test the patch tomorrow. Below is a couple of suggestions
after I read the patch. The first is - this new binary needs a manpage
:-)
+errno_t init_context(struct cache_tool_ctx **tctx, int argc, const
char *argv[]);
It is a convention that an output parameter is the last one.
+ if (tctx->user_filter) {
+ ret = sysdb_search_users(tctx, sysdb, NULL, tctx->user_filter, attrs,
+ &msg_count, &msgs);
+ if (ret != EOK) {
+ DEBUG(3, ("Finding users with filter %s failed\n",
tctx->user_filter));
I'm not a native speaker, but I think "Searching" is commonly used as a
verb, finding is usually a noun.
+ }
+
+ for (j=0 ; j<msg_count ; j++) {
+ c_name = ldb_msg_find_attr_as_string(msgs[j], SYSDB_NAME, NULL);
+ if (c_name == NULL) {
+ DEBUG(1, ("Something bad happenned, can't find attribute
%s",
+ SYSDB_NAME));
+ } else {
+ ret = sysdb_store_user(tctx, sysdb, NULL, c_name, NULL, 0, 0,
+ NULL, NULL, NULL, NULL, NULL, 0);
+ if (ret != EOK) {
+ DEBUG(5, ("Couldn't invalidate user %s",
c_name));
+ }
+ }
+ }
+ talloc_free(msgs);
+ }
While I think it may make sense to continue with invalidating groups or
netgroups even if user fails, the program should print a translatable
error message with the ERROR() macro.
+ sys_attrs = sysdb_new_attrs(tctx);
+ if (sys_attrs) {
+ ret = sysdb_attrs_add_time_t(sys_attrs,
+ SYSDB_CACHE_EXPIRE, 0);
+ if (ret == EOK) {
+ ret = sysdb_set_netgroup_attr(sysdb, NULL, c_name,
+ sys_attrs,
+ SYSDB_MOD_REP);
+ if (ret != EOK) {
+ DEBUG(3, ("Could not invalidate netgroup
%s\n",
+ c_name));
+ }
+ } else {
+ DEBUG(3, ("Could not add expiration time to "
+ "netgroup attributes\n"));
+ }
+ talloc_zfree(sys_attrs);
+ } else {
+ DEBUG(1, ("Could not create sysdb attributes\n"));
Allocation error is a serious one and should be handled.
+ ret = set_locale();
+ if (ret != EOK) {
+ DEBUG(1, ("set_locale failed (%d): %s\n", ret, strerror(ret)));
+ ERROR("Error setting the locale\n");
+ ret = EXIT_FAILURE;
Please return errno codes from the function.
+ goto fini;
+ }
+
+ pc = poptGetContext(NULL, argc, argv, long_options, 0);
+ while ((ret = poptGetNextOpt(pc)) > 0) {
+ switch (ret) {
+ case 'u':
+ idb |= INVALIDATE_USERS;
+ break;
+ case 'g':
+ idb |= INVALIDATE_GROUPS;
+ break;
+ case 'n':
+ idb |= INVALIDATE_NETGROUPS;
+ break;
+ }
+ }
+ if (ret != -1) {
+ BAD_POPT_PARAMS(pc, poptStrerror(ret), ret, fini);
+ }
+
+ debug_level = debug;
+ debug_prg_name = argv[0];
+ CHECK_ROOT(ret, debug_prg_name);
+
+ ctx = talloc_zero(NULL, struct cache_tool_ctx);
+ if (ctx == NULL) {
+ DEBUG(1, ("Could not allocate memory for tools context\n"));
+ ret = ENOMEM;
+ goto fini;
+ }
+
+ if (idb & INVALIDATE_USERS) {
+ ctx->user_filter = talloc_strdup(ctx, "");
I think it is more readable to use (SYSDB_NAME=*)
+
+fini:
+ poptFreeContext(pc);
+ if (user) free(user);
+ if (group) free(group);
+ if (netgroup) free(netgroup);
+ if (domain) free(domain);
The ifs are reduntant but harmless since the variables are initialized
to NULL.
+ if (ret != EOK && ctx) {
+ talloc_zfree(ctx);
+ }
+ *tctx = ctx;
I don't think the function should change contents of *tctx unless it is
successfull.