On Wed, Apr 13, 2011 at 04:09:32PM +0200, Jan Zelený wrote:
> From: Jan Zeleny <jzel...@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 <jzel...@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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
