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 07:32:36PM +0200, Jakub Hrozek wrote:

...

New versions attached, but only patch 6 is changed.

bye, Sumit

I don't have any more comments. Ack.

Pushed to master.