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?
+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.
+ if (subreq == NULL) {
+ ret = ENOMEM;
+ goto immediately;
+ }
+
bye,
Sumit