2:06 p.m.
Hi,
this is the first part of patches to add SID lookups, i.e. lookups by
SID and lookups for SIDs which is tracked by ticket
https://fedorahosted.org/sssd/ticket/1559 and described in
https://fedorahosted.org/sssd/wiki/DesignDocs/NSSResponderIDMappingCalls.
I split the patches into different parts to make review eaiser
(hopefully :-). This first part contains patches with minor code changes
and some new help functions. Details can be found in the commit
messages.
bye,
Sumit
2:08 p.m.
On Mon, Apr 29, 2013 at 09:06:39PM +0200, Sumit Bose wrote:
Hi,
this is the first part of patches to add SID lookups, i.e. lookups by
SID and lookups for SIDs which is tracked by ticket
https://fedorahosted.org/sssd/ticket/1559 and described in
https://fedorahosted.org/sssd/wiki/DesignDocs/NSSResponderIDMappingCalls.
I split the patches into different parts to make review eaiser
(hopefully :-). This first part contains patches with minor code changes
and some new help functions. Details can be found in the commit
messages.
bye,
Sumit
[PATCH 01/12] Remove unused TALLOC_CTX from responder_get_domain()
Ack
[PATCH 02/12] responder_get_domain: do not return disabled domains
Ack, we already do the same in find_subdomain_by_name, so it makes sense
to skip the disabled domains in responder_get_domain, too.
[PATCH 03/12] responder_get_domain(): remove timeout calculation
Wasn't the purpose of the code to assign "ret_dom = dom"; only after
establishing that the domain is not expired? That way the expired
domains would be skipped.
But I agree the current responder code can't really hit this situation
because sss_dp_get_domains_send() would check using check_last_request()
and the subdomains would be refreshed before reaching responder_get_domain()
[PATCH 04/12] LDAP: always store SID if available
Ack
[PATCH 05/12] Add secid filter to responder-dp protocol
Ack but the nested if-else structure in sss_dp_get_account_msg() is
starting to look unreadable :-) Maybe we could split it into smaller
functions in a follow up patch.
[PATCH 06/12] Add two new request types to the data-provider interface
The functionality is good, but the names are not clear to me. What about
BE_REQ_BY_SECID and BE_REQ_GET_SECID ?
[PATCH 07/12] Add idmap context to nss context
Ack
[PATCH 08/12] Add responder_get_domain_by_id()
Why does responder_get_domain_by_id() calculate the timeout but
responder_get_domain() does not?
The code itself looks good to me.
[PATCH 09/12] sysdb: add sysdb_search_object_by_sid()
Ack
[PATCH 10/12] Add sss_ncache_set_sid() and sss_ncache_check_sid()
Ack
[PATCH 11/12] Remove unused attribute list
Ack
[PATCH 12/12] Use struct to hold different types of request
I like the idea. One question though:
switch (entry_type) {
case BE_REQ_USER:
- if (name != NULL) {
+ if (req_input->type == REQ_INP_NAME) {
ret = ber_printf(ber, "{ee{ss}}", INP_NAME, REQ_FULL,
- domain_name, name);
+ domain_name,
+ req_input->inp.name);
} else {
ret = ber_printf(ber, "{ee{si}}", INP_POSIX_UID, REQ_FULL,
- domain_name, id);
+ domain_name,
+ req_input->inp.id);
Here REQ_INP_SECID would match, too, but I don't think secid would be
set. I think it would be safer to:
if (req_input->type == REQ_INP_NAME) {
/* Use req_input->inp.name */
} else if (req_input->type == REQ_INP_NAME) {
/* Use req_input->inp.id */
} else {
/* Error */
}
But maybe it would be handled in a follow up patch?
6:57 a.m.
On Wed, May 01, 2013 at 09:08:47PM +0200, Jakub Hrozek wrote:
On Mon, Apr 29, 2013 at 09:06:39PM +0200, Sumit Bose wrote:
> Hi,
>
> this is the first part of patches to add SID lookups, i.e. lookups by
> SID and lookups for SIDs which is tracked by ticket
> https://fedorahosted.org/sssd/ticket/1559 and described in
> https://fedorahosted.org/sssd/wiki/DesignDocs/NSSResponderIDMappingCalls.
>
> I split the patches into different parts to make review eaiser
> (hopefully :-). This first part contains patches with minor code changes
> and some new help functions. Details can be found in the commit
> messages.
>
> bye,
> Sumit
thank you for the review, comments a re in-line.
[PATCH 01/12] Remove unused TALLOC_CTX from responder_get_domain()
Ack
[PATCH 02/12] responder_get_domain: do not return disabled domains
Ack, we already do the same in find_subdomain_by_name, so it makes sense
to skip the disabled domains in responder_get_domain, too.
[PATCH 03/12] responder_get_domain(): remove timeout calculation
Wasn't the purpose of the code to assign "ret_dom = dom"; only after
establishing that the domain is not expired? That way the expired
domains would be skipped.
yes, but if I read the old code correctly, there is always a domain
return. And currently the callers do not check for new domains if NULL
is returned. I think it would be code to clean this up, but I think it
can wait after the release.
But I agree the current responder code can't really hit this
situation
because sss_dp_get_domains_send() would check using check_last_request()
and the subdomains would be refreshed before reaching responder_get_domain()
[PATCH 04/12] LDAP: always store SID if available
Ack
[PATCH 05/12] Add secid filter to responder-dp protocol
Ack but the nested if-else structure in sss_dp_get_account_msg() is
starting to look unreadable :-) Maybe we could split it into smaller
functions in a follow up patch.
yes, I was also thinking if a struct with type and a union would help
here to make the code more readable, but I thought here as well that it
can wait after the release.
[PATCH 06/12] Add two new request types to the data-provider interface
The functionality is good, but the names are not clear to me. What about
BE_REQ_BY_SECID and BE_REQ_GET_SECID ?
I changed BE_REQ_SECID->BE_REQ_BY_SECID to make the purpose more clear.
I would like to keep BE_REQ_USER_AND_GROUP because although it will be
used when a SID should be lookup up, the general purpose is the check
the users and the groups for the given name or id.
[PATCH 07/12] Add idmap context to nss context
Ack
[PATCH 08/12] Add responder_get_domain_by_id()
Why does responder_get_domain_by_id() calculate the timeout but
responder_get_domain() does not?
The code itself looks good to me.
See above.
[PATCH 09/12] sysdb: add sysdb_search_object_by_sid()
Ack
[PATCH 10/12] Add sss_ncache_set_sid() and sss_ncache_check_sid()
Ack
[PATCH 11/12] Remove unused attribute list
Ack
[PATCH 12/12] Use struct to hold different types of request
I like the idea. One question though:
> switch (entry_type) {
> case BE_REQ_USER:
> - if (name != NULL) {
> + if (req_input->type == REQ_INP_NAME) {
> ret = ber_printf(ber, "{ee{ss}}", INP_NAME, REQ_FULL,
> - domain_name, name);
> + domain_name,
> + req_input->inp.name);
> } else {
> ret = ber_printf(ber, "{ee{si}}", INP_POSIX_UID,
REQ_FULL,
> - domain_name, id);
> + domain_name,
> + req_input->inp.id);
Here REQ_INP_SECID would match, too, but I don't think secid would be
set. I think it would be safer to:
if (req_input->type == REQ_INP_NAME) {
/* Use req_input->inp.name */
} else if (req_input->type == REQ_INP_NAME) {
/* Use req_input->inp.id */
} else {
/* Error */
}
But maybe it would be handled in a follow up patch?
yes, it handled in the patch which adds support for REQ_INP_SECID to
the IPA subdomains.
New versions attached, but only patch 6 is changed.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
12:32 p.m.
On Thu, May 02, 2013 at 01:57:29PM +0200, Sumit Bose wrote:
On Wed, May 01, 2013 at 09:08:47PM +0200, Jakub Hrozek wrote:
> On Mon, Apr 29, 2013 at 09:06:39PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > this is the first part of patches to add SID lookups, i.e. lookups by
> > SID and lookups for SIDs which is tracked by ticket
> > https://fedorahosted.org/sssd/ticket/1559 and described in
> > https://fedorahosted.org/sssd/wiki/DesignDocs/NSSResponderIDMappingCalls.
> >
> > I split the patches into different parts to make review eaiser
> > (hopefully :-). This first part contains patches with minor code changes
> > and some new help functions. Details can be found in the commit
> > messages.
> >
> > bye,
> > Sumit
thank you for the review, comments a re in-line.
>
> [PATCH 01/12] Remove unused TALLOC_CTX from responder_get_domain()
> Ack
>
> [PATCH 02/12] responder_get_domain: do not return disabled domains
> Ack, we already do the same in find_subdomain_by_name, so it makes sense
> to skip the disabled domains in responder_get_domain, too.
>
> [PATCH 03/12] responder_get_domain(): remove timeout calculation
> Wasn't the purpose of the code to assign "ret_dom = dom"; only after
> establishing that the domain is not expired? That way the expired
> domains would be skipped.
yes, but if I read the old code correctly, there is always a domain
return. And currently the callers do not check for new domains if NULL
is returned. I think it would be code to clean this up, but I think it
can wait after the release.
Agreed:
https://fedorahosted.org/sssd/ticket/1902
> But I agree the current responder code can't really hit this
situation
> because sss_dp_get_domains_send() would check using check_last_request()
> and the subdomains would be refreshed before reaching responder_get_domain()
>
> [PATCH 04/12] LDAP: always store SID if available
> Ack
>
> [PATCH 05/12] Add secid filter to responder-dp protocol
> Ack but the nested if-else structure in sss_dp_get_account_msg() is
> starting to look unreadable :-) Maybe we could split it into smaller
> functions in a follow up patch.
yes, I was also thinking if a struct with type and a union would help
here to make the code more readable, but I thought here as well that it
can wait after the release.
Sure, it's not needed to do it now:
https://fedorahosted.org/sssd/ticket/1903
>
> [PATCH 06/12] Add two new request types to the data-provider interface
> The functionality is good, but the names are not clear to me. What about
> BE_REQ_BY_SECID and BE_REQ_GET_SECID ?
I changed BE_REQ_SECID->BE_REQ_BY_SECID to make the purpose more clear.
I would like to keep BE_REQ_USER_AND_GROUP because although it will be
used when a SID should be lookup up, the general purpose is the check
the users and the groups for the given name or id.
OK, works for by
>
> [PATCH 07/12] Add idmap context to nss context
> Ack
>
> [PATCH 08/12] Add responder_get_domain_by_id()
> Why does responder_get_domain_by_id() calculate the timeout but
> responder_get_domain() does not?
> The code itself looks good to me.
See above.
>
> [PATCH 09/12] sysdb: add sysdb_search_object_by_sid()
> Ack
>
> [PATCH 10/12] Add sss_ncache_set_sid() and sss_ncache_check_sid()
> Ack
>
> [PATCH 11/12] Remove unused attribute list
> Ack
>
> [PATCH 12/12] Use struct to hold different types of request
> I like the idea. One question though:
>
> > switch (entry_type) {
> > case BE_REQ_USER:
> > - if (name != NULL) {
> > + if (req_input->type == REQ_INP_NAME) {
> > ret = ber_printf(ber, "{ee{ss}}", INP_NAME,
REQ_FULL,
> > - domain_name, name);
> > + domain_name,
> > + req_input->inp.name);
> > } else {
> > ret = ber_printf(ber, "{ee{si}}", INP_POSIX_UID,
REQ_FULL,
> > - domain_name, id);
> > + domain_name,
> > + req_input->inp.id);
>
> Here REQ_INP_SECID would match, too, but I don't think secid would be
> set. I think it would be safer to:
>
> if (req_input->type == REQ_INP_NAME) {
> /* Use req_input->inp.name */
> } else if (req_input->type == REQ_INP_NAME) {
> /* Use req_input->inp.id */
> } else {
> /* Error */
> }
>
> But maybe it would be handled in a follow up patch?
yes, it handled in the patch which adds support for REQ_INP_SECID to
the IPA subdomains.
OK, then it's fine.
New versions attached, but only patch 6 is changed.
bye,
Sumit
I don't have any more comments. Ack.
4010
days inactive
4013
days old
sssd-devel@lists.fedorahosted.org
4 comments
2 participants
participants (2)
-
Jakub Hrozek -
Sumit Bose