[SSSD] [PATCHES] ad: use tokengroups even when id mapping is disabled
Pavel Březina
pbrezina at redhat.com
Thu Dec 12 12:20:59 UTC 2013
On 12/12/2013 01:08 PM, Jakub Hrozek wrote:
> On Thu, Dec 12, 2013 at 12:11:00PM +0100, Sumit Bose wrote:
>> On Thu, Dec 12, 2013 at 11:46:01AM +0100, Pavel Březina wrote:
>>> On 12/10/2013 04:59 PM, Sumit Bose wrote:
>>>> On Wed, Nov 20, 2013 at 02:41:49PM +0100, Pavel Březina wrote:
>>>>> On 11/19/2013 11:52 AM, Jakub Hrozek wrote:
>>>>>> On Fri, Nov 15, 2013 at 12:22:53PM +0100, Pavel Březina wrote:
>>>>>>> https://fedorahosted.org/sssd/ticket/1568
>>>>>>
>>>>>>> From b66343b207679cbbbdb5d4a54a7f465fbf2ec97f Mon Sep 17 00:00:00 2001
>>>>>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>>>>>> Date: Tue, 12 Nov 2013 13:52:35 +0100
>>>>>>> Subject: [PATCH 1/3] sysdb: add sysdb_group_dn_str()
>>>>>>
>>>>>> The patch doesn't apply, but more importantly there already is
>>>>>> sysdb_group_dn_str(), can you use that instead?
>>>>>
>>>>> Thanks.
>>>>>
>>>>>>
>>>>>> The other two patches don't apply either. I have just a couple of
>>>>>> comments and questions as I scrolled through them:
>>>>>>
>>>>>> Patch #2 looks good to me so far, but I only read the diff.
>>>>>>
>>>>>> In patch #3, I don't see the point of using tmp_ctx and stealing to "state"
>>>>>> later if the request is relatively short. Can you use "state" directly?
>>>>>> That way you guarantee that the memory will go away with the request.
>>>>>
>>>>> I don't know, I think freeing all resources when they become
>>>>> irrelevant is more clear.
>>>>>
>>>>>> btw did you test if the code works even if only some of the groups have
>>>>>> posix attributes set?
>>>>>
>>>>> Yes, those non-posix groups are downloaded by SID but they have
>>>>> empty members.
>>>>>
>>>>> Rebase patches are attached.
>>>>
>>>> Patches apply and work as expected. Nevertheless I have two comments.
>>>>
>>>>>
>>>>> -errno_t
>>>>> -sdap_get_ad_tokengroups_initgroups_recv(struct tevent_req *req)
>>>>> +int sdap_ad_tokengroups_initgroups_recv(struct tevent_req *req)
>>>>> {
>>>>
>>>> is there a reason to switch from errno_t to int here?
>>>
>>> None. Fixed.
>>>
>>>>> +struct tevent_req *
>>>>> +sdap_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
>>>>> + struct tevent_context *ev,
>>>>> + struct sdap_id_ctx *id_ctx,
>>>>> + struct sdap_options *opts,
>>>>> + struct sysdb_ctx *sysdb,
>>>>> + struct sss_domain_info *domain,
>>>>> + struct sdap_handle *sh,
>>>>> + const char *name,
>>>>> + const char *orig_dn,
>>>>> + int timeout,
>>>>> + bool use_id_mapping)
>>>>> +{
>>>>> + struct sdap_ad_tokengroups_initgroups_state *state = NULL;
>>>>> + struct tevent_req *req = NULL;
>>>>> + struct tevent_req *subreq = NULL;
>>>>> + errno_t ret;
>>>>> +
>>>>> + req = tevent_req_create(mem_ctx, &state,
>>>>> + struct sdap_ad_tokengroups_initgroups_state);
>>>>> + if (req == NULL) {
>>>>> + DEBUG(SSSDBG_CRIT_FAILURE, ("tevent_req_create() failed\n"));
>>>>> + return NULL;
>>>>> + }
>>>>> +
>>>>> + state->use_id_mapping = use_id_mapping;
>>>>> +
>>>>> + if (state->use_id_mapping) {
>>>>> + subreq = sdap_ad_tokengroups_initgr_mapping_send(state, ev, opts,
>>>>> + sysdb, domain, sh,
>>>>> + name, orig_dn,
>>>>> + timeout);
>>>>> + } else {
>>>>> + subreq = sdap_ad_tokengroups_initgr_posix_send(state, ev, id_ctx, opts,
>>>>> + sysdb, domain, sh,
>>>>> + name, orig_dn,
>>>>> + timeout);
>>>>> + }
>>>>
>>>> I think it would avoid a bit of code duplication if this check would be
>>>> done after the tokenGroups is read from the server.
>>>
>>> You would avoid a little code duplication, but I believe it is not
>>> worth it, because the duplication is only in calling
>>> sdap_get_ad_tokengroups_send() and the amount of glue code would be
>>> approximately the same.
>>>
>>> Also I think, the current code flow is easier to follow.
>>
>> I guess it's a matter of taste :-), nevertheless ACK.
>>
>> bye,
>> Sumit
>
> I see a number of compilation warnings with the patches (using clang-3.3)
>
> /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:785:9: warning: variable 'in_transaction' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> if (ret != EOK) {
> ^~~~~~~~~~
> /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:873:9: note: uninitialized use occurs here
> if (in_transaction) {
> ^~~~~~~~~~~~~~
> /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:785:5: note: remove the 'if' if its condition is always false
> if (ret != EOK) {
> ^~~~~~~~~~~~~~~~~
> /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:778:9: warning: variable 'in_transaction' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> if (groups == NULL) {
> ^~~~~~~~~~~~~~
I also seen following warning with gcc and higher optimization level:
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:
In function 'sdap_ad_tokengroups_initgr_posix_tg_done':
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:1008:20:
warning: 'num_sids' may be used uninitialized in this function
[-Wmaybe-uninitialized]
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:
In function 'sdap_ad_tokengroups_initgr_mapping_done':
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:873:8:
warning: 'in_transaction' may be used uninitialized in this function
[-Wuninitialized]
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:777:14:
warning: 'num_sids' may be used uninitialized in this function
[-Wmaybe-uninitialized]
New patches attached. Here's the diff.
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -746,14 +746,14 @@ static void
sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
const char *name = NULL;
const char *sid = NULL;
char **sids = NULL;
- size_t num_sids;
+ size_t num_sids = 0;
size_t i;
time_t now;
gid_t gid;
char **groups = NULL;
size_t num_groups;
errno_t ret, sret;
- bool in_transaction;
+ bool in_transaction = false;
tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) {
@@ -977,7 +977,7 @@ sdap_ad_tokengroups_initgr_posix_tg_done(struct
tevent_req *subreq)
const char *name = NULL;
char *sid = NULL;
char **sids = NULL;
- size_t num_sids;
+ size_t num_sids = 0;
char **valid_groups = NULL;
size_t num_valid_groups;
char **missing_sids = NULL;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ad-refactor-tokengroups-initgroups.patch
Type: text/x-patch
Size: 27787 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131212/9535a130/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ad-use-tokengroups-even-when-id-mapping-is-disabled.patch
Type: text/x-patch
Size: 23643 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131212/9535a130/attachment-0001.bin>
More information about the sssd-devel
mailing list