-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 04/14/2011 01:39 PM, Jakub Hrozek wrote:
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.
You are correct, however there are many places where we are still
following this model. Please open a Trac ticket and we'll fix this
separately.
> 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.
Strictly speaking, this is correct English, but it's still awkward.
Please use "Searching". ("Finding" implies success in the search)
> + }
> +
> + 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",
Too many "n"s in "happened".
> + 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.
They're redundant no matter what, since free(NULL) is a noop.
> + if (ret != EOK && ctx) {
> + talloc_zfree(ctx);
> + }
> + *tctx = ctx;
I don't think the function should change contents of *tctx unless it is
successfull.
+1
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
- --
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAk2nMrUACgkQeiVVYja6o6PzjQCeKpttCDtxDcqYeAh+olcd+CcU
HVoAoKrgVNF1Fmr3Ba3PFB9//A+dJbFS
=ZguP
-----END PGP SIGNATURE-----