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(a)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;