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
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?
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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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.
sssd-devel@lists.fedorahosted.org