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