On Thu, Feb 12, 2015 at 10:07:57AM +0100, Pavel Březina wrote:
>I'm not sure why we have both enum cache_req_type and
sss_dp_acct_type
>with 1:1 mapping used at the same abstraction layer. Either
>cache_req_type is something private to the cache_req code and then we
>should always use it except when sending requests to the DP where we
>should do a one-time conversion, or we can always use sss_dp_acct_type.
>
>Maybe add another patch that replaces dp_type with type for internal
>branching within this module?
The cache_req_type is used to distinguish between get_user_by_name,
get_user_by_id, get_initgroups call which all maps to DP_USER call to
provider. So the sss_dp_acct_type is not sufficient because we need larger
granularity but at the same time we can't get rid of if that easily unless
the dp interface is changed. This may be a future enhancement when other
responders use this.
OK, can you add a note to the code that the member should be removed
later? Just a FIXME would do.
[...]
>In general I like the approach you've taken but I think we
can make it
>even better. Add another structure like cache_req_dom_ctx and add it inside
>the cache_req_input structure. Always create cache_req_dom_ctx as a
>talloc child of cache_req_input.
>
>This would not only collapse the above talloc_frees into one, but it
>would also nicely separate the per-domain data from per-lookup data and
>avoid mixing data from different domains.
>
>I also think dom_name sounds better than safe_name, but meh.
This is my main point in the whole patchset, I think.
>
>>+
>>+ switch (input->type) {
>>+ case CACHE_REQ_USER_BY_NAME:
>>+ case CACHE_REQ_INITGROUPS:
>>+ name = sss_get_cased_name(tmp_ctx, input->orig_name,
>>+ domain->case_sensitive);
>>+ if (name == NULL) {
>>+ ret = ENOMEM;
>>+ goto done;
>>+ }
>>+
>>+ name = sss_reverse_replace_space(tmp_ctx, name,
rctx->override_space);
>>+ if (name == NULL) {
>>+ ret = ENOMEM;
>>+ goto done;
>>+ }
>>+
>>+ fqn = talloc_asprintf(tmp_ctx, "%s@%s", name,
domain->name);
>
>I'm afraid FQDN is configurable in SSSD, so creating it is not this easy..
This is just for purpose of debug messages, so its ok imho. It is used in
sssd everywhere.
OK, then would it be too much work to rename the variable to something
like log_fqn or debug_fqn? My concern is that 2 years later someone
would use this data for output, not realizing the data is hardcoded.
[...]
>> static errno_t cache_req_get_object(TALLOC_CTX *mem_ctx,
>>- enum sss_dp_acct_type dp_type,
>>- struct sss_domain_info *domain,
>>- const char *name,
>>+ struct cache_req_input *input,
>> struct ldb_result **_result)
>> {
>> struct ldb_result *result = NULL;
>> bool one_item_only;
>> errno_t ret;
>>
>>- DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s@%s]\n",
>>- name, domain->name);
>>+ DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s]\n",
input->fqn);
>>
>>- switch (dp_type) {
>>- case SSS_DP_USER:
>>+ switch (input->type) {
>>+ case CACHE_REQ_USER_BY_NAME:
>> one_item_only = true;
>>- ret = sysdb_getpwnam_with_views(mem_ctx, domain, name, &result);
>>+ ret = sysdb_getpwnam_with_views(mem_ctx, input->domain,
>>+ input->safe_name, &result);
>> break;
>>- case SSS_DP_INITGROUPS:
>>+ case CACHE_REQ_INITGROUPS:
>> one_item_only = false;
>>- ret = sysdb_initgroups_with_views(mem_ctx, domain, name, &result);
>>+ ret = sysdb_initgroups_with_views(mem_ctx, input->domain,
>>+ input->safe_name, &result);
>> break;
>> default:
>> ret = EINVAL;
>>- DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported DP request type\n");
>>+ DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request
type\n");
>> break;
>
>Should we free result if ret != EOK ?
Hmm, probably yes.
>This is as far as I've got today, I'll continue with review tomorrow.
I will send a review of the other patches separately.