[PATCH] Cache cleaning utility

[Jan Zelený]

I'm sending three patches with the cache-cleaning utility and some updates in 
sysdb which this utility depends on.

Thanks
Jan

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/12/2011 04:41 AM, Jan Zelený wrote:
> I'm sending three patches with the cache-cleaning utility and some updates in 
> sysdb which this utility depends on.

I haven't done a full review of this feature yet, but at a glance I
notice that it doesn't have support for expiring netgroups. This should
be added.

- -- 
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/
>
> iEYEARECAAYFAk2kUV0ACgkQeiVVYja6o6MD7ACZAbswNUJIg5Sm+jxhvi9Va6wK
> 6I0An3lGlnTzIdhZABh1iTmYaSxoEvuX
> =4CO/
> -----END PGP SIGNATURE-----

[Jan Zelený]

Stephen Gallagher  wrote:
> On 04/12/2011 04:41 AM, Jan Zelený wrote:
> > I'm sending three patches with the cache-cleaning utility and some
> > updates in sysdb which this utility depends on.
>
> I haven't done a full review of this feature yet, but at a glance I
> notice that it doesn't have support for expiring netgroups. This should
> be added.

Done. Patches in attachment.

Jan

[Jakub Hrozek]

On Wed, Apr 13, 2011 at 04:09:32PM +0200, Jan Zelený wrote:
> From: Jan Zeleny 
> 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 
> 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.

[Stephen Gallagher]

-----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 
> > 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 
> > 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
> [e-mail]
> 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-----

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/14/2011 07:45 PM, Stephen Gallagher wrote:
> 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.

https://fedorahosted.org/sssd/ticket/848
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAk2oDLEACgkQHsardTLnvCUMRQCgiIVnzY0i22CIXgDR4Va4WY5+
> YbAAn2H3MgkuAI7YET6iG6CCsUVpIYxD
> =AQjQ
> -----END PGP SIGNATURE-----

[Jeff Schroeder]

On Thu, Apr 14, 2011 at 10:39 AM, Jakub Hrozek  wrote:
... snip ...
> > 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.

My 0.2c:
a noun is a person, place, or thing. As a general rule, anything
ending with "ing" that is something you can do is a verb. This
includes finding. However, "Searching for" is probably better than
"Finding" in this context.

[Jan Zelený]

I'm sending corrected patch. I have one comment though.

Jakub Hrozek  wrote:
> Allocation error is a serious one and should be handled.

I don't think we should handle it differently (how?). After all it is handled 
the same way like if allocation error occurred let's say in 
sysdb_search_netgroups().

Jan

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/15/2011 07:22 PM, Jan Zeleny wrote:
> I'm sending corrected patch. I have one comment though.
>
> Jakub Hrozek  wrote:
> > Allocation error is a serious one and should be handled.
>
> I don't think we should handle it differently (how?). After all it is handled 
> the same way like if allocation error occurred let's say in 
> sysdb_search_netgroups().
>
> Jan

I think that if an OOM situation occurs, we should really just clean up
and quit gracefully - in this case, do a sysdb_transaction_cancel();
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAk2r614ACgkQHsardTLnvCX4OQCg1iH/2SKuoKU1UO+8ja/VPnpE
> Zc8AoJsqWQJbOLBOteZuZpDKzLWwzVwZ
> =0Ij2
> -----END PGP SIGNATURE-----

[Jan Zelený]

> On 04/15/2011 07:22 PM, Jan Zeleny wrote:
> > I'm sending corrected patch. I have one comment though.
> >
> > Jakub Hrozek  wrote:
> > > Allocation error is a serious one and should be handled.
> >
> > I don't think we should handle it differently (how?). After all it is
> > handled the same way like if allocation error occurred let's say in
> > sysdb_search_netgroups().
> >
> > Jan
>
> I think that if an OOM situation occurs, we should really just clean up
> and quit gracefully - in this case, do a sysdb_transaction_cancel();

Yes, but then we'd need to do this check after each call of a procedure which 
might end up the same way, which is exactly what I'd like to avoid.

Jan

[Jan Zelený]

Jan Zelený  wrote:
> > On 04/15/2011 07:22 PM, Jan Zeleny wrote:
> > > I'm sending corrected patch. I have one comment though.
> > >
> > > Jakub Hrozek  wrote:
> > > > Allocation error is a serious one and should be handled.
> > >
> > > I don't think we should handle it differently (how?). After all it is
> > > handled the same way like if allocation error occurred let's say in
> > > sysdb_search_netgroups().
> > >
> > > Jan
> >
> > I think that if an OOM situation occurs, we should really just clean up
> > and quit gracefully - in this case, do a sysdb_transaction_cancel();
>
> Yes, but then we'd need to do this check after each call of a procedure
> which might end up the same way, which is exactly what I'd like to avoid.
>
> Jan

Here is a complete set of patches which are needed for the review.

Jan

[Stephen Gallagher]

On Wed, 2011-04-20 at 14:24 +0200, Jan Zeleny wrote:
> Here is a complete set of patches which are needed for the review.

jzeleny-013-2-sysdb-changes-for-sss_cache.patch:
Nack.

I'd prefer if you used

list->num_dbs = 1;
list->dbs = talloc_array(list, struct sysdb_ctx, list->num_dbs);

so it's clear from the code that you're creating an array of one
element.

Also, please split this into two patches. The sysdb_list_init() and
sysdb_search_netgroups() routines are not logically related and should
be separate patches.

jzeleny-014-3-cache-cleaning-utility.patch:
Nack.

For readability, please use spaces around conditional operators such as
less-than '<'

After sysdb_search_users(), if it failed, we should 'continue;'. We
should not fall into the msg_count loop which is uninitialized.

If we can't find SYSDB_NAME in the msg, that should also throw 
ERROR("Couldn't invalidate user"); //No name available

Using sysdb_store_[user|group]() is complete overkill. We don't want to
be updating the user. This will actually set the expiration to "now + 0"
which means that you've just introduced a race condition for one second.
You will want to use sysdb_set_[user|group]_attr() and force the
expiration time to the value '1'.

Expiration time 0 means "never expire", so your netgroup expiration is
not going to work at all. Set it to '1'.

If the sysdb_transaction_commit() fails, you MUST attempt to call
sysdb_transaction_cancel() as well.

jzeleny-015-2-minor-fixes-in-sysdb.patch:
Ack

jzeleny-017-man-page-for-cache-cleanup.patch
Ack

[Stephen Gallagher]

On Wed, 2011-04-20 at 09:59 -0400, Stephen Gallagher wrote:
> On Wed, 2011-04-20 at 14:24 +0200, Jan Zeleny wrote:
> jzeleny-014-3-cache-cleaning-utility.patch:
> Nack.

One additional comment. I think it's fine for us to just skip over any
failures and keep trying, because this is a non-destructive operation.
Our worst-case scenario is that some entries stay in the cache too long.

As long as we print an error message for any entry that was skipped, I
think it's acceptable.

[Jan Zelený]

Ok, this is corrected set of patches. All your comments were taken into 
account. Also please note that jzeleny-015-2-minor-fixes-in-sysdb.patch which 
was acked before has been updated.
> On Wed, 2011-04-20 at 14:24 +0200, Jan Zeleny wrote:
> > Here is a complete set of patches which are needed for the review.
>
> jzeleny-013-2-sysdb-changes-for-sss_cache.patch:
> Nack.
>
> I'd prefer if you used
>
> list->num_dbs = 1;
> list->dbs = talloc_array(list, struct sysdb_ctx, list->num_dbs);
>
> so it's clear from the code that you're creating an array of one
> element.
>
> Also, please split this into two patches. The sysdb_list_init() and
> sysdb_search_netgroups() routines are not logically related and should
> be separate patches.
>
> jzeleny-014-3-cache-cleaning-utility.patch:
> Nack.
>
> For readability, please use spaces around conditional operators such as
> less-than '<'
>
> After sysdb_search_users(), if it failed, we should 'continue;'. We
> should not fall into the msg_count loop which is uninitialized.
>
> If we can't find SYSDB_NAME in the msg, that should also throw
> ERROR("Couldn't invalidate user"); //No name available
>
> Using sysdb_store_[user|group]() is complete overkill. We don't want to
> be updating the user. This will actually set the expiration to "now + 0"
> which means that you've just introduced a race condition for one second.
> You will want to use sysdb_set_[user|group]_attr() and force the
> expiration time to the value '1'.
>
> Expiration time 0 means "never expire", so your netgroup expiration is
> not going to work at all. Set it to '1'.
>
> If the sysdb_transaction_commit() fails, you MUST attempt to call
> sysdb_transaction_cancel() as well.
>
> jzeleny-015-2-minor-fixes-in-sysdb.patch:
> Ack
>
> jzeleny-017-man-page-for-cache-cleanup.patch
> Ack

[Stephen Gallagher]

On Tue, 2011-05-03 at 11:30 +0200, Jan Zelený wrote:
> Ok, this is corrected set of patches. All your comments were taken into 
> account. Also please note that jzeleny-015-2-minor-fixes-in-sysdb.patch which 
> was acked before has been updated.
> > On Wed, 2011-04-20 at 14:24 +0200, Jan Zeleny wrote:
> > > Here is a complete set of patches which are needed for the review.
> >
> > jzeleny-013-2-sysdb-changes-for-sss_cache.patch:
> > Nack.
> >
> > I'd prefer if you used
> >
> > list->num_dbs = 1;
> > list->dbs = talloc_array(list, struct sysdb_ctx, list->num_dbs);
> >
> > so it's clear from the code that you're creating an array of one
> > element.
> >
> > Also, please split this into two patches. The sysdb_list_init() and
> > sysdb_search_netgroups() routines are not logically related and should
> > be separate patches.
> >
> > jzeleny-014-3-cache-cleaning-utility.patch:
> > Nack.
> >
> > For readability, please use spaces around conditional operators such as
> > less-than '<'
> >
> > After sysdb_search_users(), if it failed, we should 'continue;'. We
> > should not fall into the msg_count loop which is uninitialized.
> >
> > If we can't find SYSDB_NAME in the msg, that should also throw
> > ERROR("Couldn't invalidate user"); //No name available
> >
> > Using sysdb_store_[user|group]() is complete overkill. We don't want to
> > be updating the user. This will actually set the expiration to "now + 0"
> > which means that you've just introduced a race condition for one second.
> > You will want to use sysdb_set_[user|group]_attr() and force the
> > expiration time to the value '1'.
> >
> > Expiration time 0 means "never expire", so your netgroup expiration is
> > not going to work at all. Set it to '1'.
> >
> > If the sysdb_transaction_commit() fails, you MUST attempt to call
> > sysdb_transaction_cancel() as well.
> >
> > jzeleny-015-2-minor-fixes-in-sysdb.patch:
> > Ack
> >
> > jzeleny-017-man-page-for-cache-cleanup.patch
> > Ack

Ack. Good work.

[Stephen Gallagher]

On Wed, 2011-05-04 at 10:44 -0400, Stephen Gallagher wrote:
> On Tue, 2011-05-03 at 11:30 +0200, Jan Zelený wrote:
> > Ok, this is corrected set of patches. All your comments were taken into 
> > account. Also please note that jzeleny-015-2-minor-fixes-in-sysdb.patch which 
> > was acked before has been updated.
> > > On Wed, 2011-04-20 at 14:24 +0200, Jan Zeleny wrote:
> > > > Here is a complete set of patches which are needed for the review.
> > >
> > > jzeleny-013-2-sysdb-changes-for-sss_cache.patch:
> > > Nack.
> > >
> > > I'd prefer if you used
> > >
> > > list->num_dbs = 1;
> > > list->dbs = talloc_array(list, struct sysdb_ctx, list->num_dbs);
> > >
> > > so it's clear from the code that you're creating an array of one
> > > element.
> > >
> > > Also, please split this into two patches. The sysdb_list_init() and
> > > sysdb_search_netgroups() routines are not logically related and should
> > > be separate patches.
> > >
> > > jzeleny-014-3-cache-cleaning-utility.patch:
> > > Nack.
> > >
> > > For readability, please use spaces around conditional operators such as
> > > less-than '<'
> > >
> > > After sysdb_search_users(), if it failed, we should 'continue;'. We
> > > should not fall into the msg_count loop which is uninitialized.
> > >
> > > If we can't find SYSDB_NAME in the msg, that should also throw
> > > ERROR("Couldn't invalidate user"); //No name available
> > >
> > > Using sysdb_store_[user|group]() is complete overkill. We don't want to
> > > be updating the user. This will actually set the expiration to "now + 0"
> > > which means that you've just introduced a race condition for one second.
> > > You will want to use sysdb_set_[user|group]_attr() and force the
> > > expiration time to the value '1'.
> > >
> > > Expiration time 0 means "never expire", so your netgroup expiration is
> > > not going to work at all. Set it to '1'.
> > >
> > > If the sysdb_transaction_commit() fails, you MUST attempt to call
> > > sysdb_transaction_cancel() as well.
> > >
> > > jzeleny-015-2-minor-fixes-in-sysdb.patch:
> > > Ack
> > >
> > > jzeleny-017-man-page-for-cache-cleanup.patch
> > > Ack
>
> Ack. Good work.

Pushed to master.