[SSSD] [PATCH] Save dummy groups to cache during initgroups (master)

Stephen Gallagher sgallagh at redhat.com
Mon Oct 4 12:23:59 UTC 2010


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

On 09/30/2010 08:38 AM, Jakub Hrozek wrote:
> [PATCH 1/2] sysdb interface for adding fake groups
> Adds a sysdb_add_fake_group() call that adds an expired group entry.
> 
> Also tweaks the NSS code a little so the fake group entries that are
> invalid are skipped. This is not how we use the fake groups in patch #2
> as we also store the gid, but I think it's worth making sure we don't
> return fake groups now that we have a mechanism to add them.

Nack.
I keep forgetting to add this to our formal coding specification, but
please do not pass a mem_ctx into any function that does not return an
allocated value.

The tmp_ctx should be allocated on NULL so that it would turn up on a
valgrind report if it leaks. If it's attached to mem_ctx, then it will
still be available and undetectable until that parent context is freed.

(In the 1.2 version, we'll probably need this memory context, since
we'll be dealing with sysdb as a tevent subrequest)



> [PATCH 2/2] Save dummy groups to cache during initgroups
> When performing initgroups/getgrouplist on RFC2307, add fake group
> entries for those groups that are not cached already. That way, complete
> group membership is returned without saving complete group objects.
> 
> These two patches apply to master only but I have a 1.2 counterpart -
> currently only in my fedorapeople repo - I can resend them when/if these
> are approved as these read much easily, so I think it makes sense to
> review them first.

Nack.

Same comment as above with the memory context.

We shouldn't return EINVAL when we get zero groups. We should just
short-circuit and return EOK, since there's nothing to be done.

Personal preference: please separate missing[mi] and mi++ in the code.
It reads easier as separate lines.

You need to initialize in_transaction to false, or error cases before
the transaction start will be wrong.

If all groups are cached, you're leaking memory by not freeing tmp_ctx;

I'm not sure why you're searching the sysdb for groups here. It's been
done by sdap_initgr_rfc2307_process(). We're only passing in the list of
entries that are missing from sysdb. If you're worried that the sysdb
has been changed since it was previously checked, then you probably want
to wrap the lookup in a transaction, not just the write.


If the missing group is not found in ldap_groups, that should throw an
error.

- -- 
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.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkypx14ACgkQeiVVYja6o6N/ggCcCQGKd9Ux/d0o3U025F2K0hnb
HfwAn2NnGwEJ+9VmWIXnRUIwPkIdNDZU
=KNjr
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list