On 07/13/2015 06:25 PM, Jakub Hrozek wrote:
On Fri, Jul 10, 2015 at 01:30:07PM +0200, Pavel Březina wrote:
> On 07/09/2015 12:17 PM, Jakub Hrozek wrote:
>>> It may be also better
>>> to move them into the sysdb module?
>>
>> I would do that if we anticipate more listing intefaces using this
>> kind of filter.
>
> I disagree since in my opinion it belongs into sysdb, but I will not insist.
>
>>> If I read the code correctly, you always return only records that were
>>> updated. It this sufficient? I think we need to return all records that
>>> match the provided filter.
>>
>> Please check my logic here, but I think the way it's coded now is
>> safer. On the back end side, we can't reasonably detect if an entry has
>> been removed if we only use the wildcard lookup. So the back end can't
>> reasonably decide which entries have been removed from the server.
>>
>> The reason is the limit. There might be more entries on the server side
>> that those that match the filter, so what the back end sees is just a
>> subset. And the entries outside the limit can be cached by direct
>> lookups, so we simply shouldn't touch them..
>>
>> What the code does now is only returns entries updated by the wildcard
>> lookup. The entries that were not updated are not returned to the
>> fronted -- they just sit in the cache until some application requests
>> them with a direct lookup. The direct lookup can detect removal of the
>> object on the server side and also remove the cached object.
>>
>> Does that make sense?
>
> Yes, thank you for explanation. I think the logic is sound.
>
>>>> if (state->result == NULL || state->result->count == 0 ||
>>>> state->input->type == CACHE_REQ_USER_BY_FILTER ||
>>>> state->input->type == CACHE_REQ_GROUP_BY_FILTER) {
>>>> ret = ENOENT;
>>>> } else {
>>>
>>> I would like to avoid this type of code whenever possible. Can we replace it
>>> with a macro e.g. always_check_cache(state->input) or function?
>>
>> OK, done. Please feel free to suggest a better function name.
>>
>>>
>>> Is it a valid operation to use a filter that needs to be parse into name and
>>> domain (user*@domain)?
>>
>> I don't think so, we have a specialized function for that.
ListByDomainAndName
Ah, ok.
>
> What function?
>
> *Patch #01 tests: Move N_ELEMENTS definition to tests/common.h*
> ACK
>
> *Patch #02 SYSDB: Add functions to look up multiple entries including name
> and custom filter*
>
>> +static char *enum_filter(TALLOC_CTX *mem_ctx,
>> + const char *base_filter,
>> + const char *name_filter,
>> + const char *addtl_filter)
>
> You are leaking memory here if any of the allocation fails. I know it will
> be freed in the caller but it is not a good practice.
As discussed on IRC, I added a context.
Nack. You added a tmp_ctx but you still use mem_ctx :-)
>
> *Patch #03 DP: Add DP_WILDCARD and
> SSS_DP_WILDCARD_USER/SSS_DP_WILDCARD_GROUP*
>
>> + } else if (info->type == SSS_DP_WILDCARD_USER ||
>> + info->type == SSS_DP_WILDCARD_GROUP) {
>> + if (info->extra) {
>> + filter = talloc_asprintf(info, "%s=%s:%s",
DP_WILDCARD,
>> + info->opt_name,
info->extra);
>> + } else {
>> + filter = talloc_asprintf(info, "%s=%s", DP_WILDCARD,
>> + info->opt_name);
>> + }
>
> Do you think it is safe to use the same prefix for both users and groups
> look up?
yes, because the target is defined with be_type.
Ok. Ack.
>
> *Patch #04 cache_req: Extend cache_req with wildcard lookups*
>
>> case CACHE_REQ_USER_BY_FILTER:
>> case CACHE_REQ_GROUP_BY_FILTER:
>> /* Nothing to do, adding a wildcard request to ncache doesn't
>> * make sense */
>
>> /* Return true if the request can be cached or false if the cache_req
>> * code needs to contact the DP every time
>> */
>
> Can you finish the sentence with dot so it is consistent with the rest of
> the code?
Done.
>
>> static bool cache_req_cachable(struct cache_req_input *input)
>
> How about cache_req_avoid_cache?
OK, renamed.
>
> *Patch #05 UTIL: Add sss_filter_sanitize_ex*
>
>> + const char has_all_allow_asterisk_expected[] =
"\\5c\\28user\\29*name";
>
> Can you add comment with unescaped characters so it is human-readable?
Done.
>
> *Patch #06 LDAP: Fetch users and groups using wildcards*
>
>> @@ -953,6 +991,14 @@ static void groups_get_done(struct tevent_req *subreq)
>> * group we have nothing to do here. */
>> break;
>>
>> + case BE_FILTER_WILDCARD:
>> + /* We can't know if all users are up-to-date, especially in a
large
>> + * environment. Do not delete any records, let the responder fetch
>> + * the entries they are requested in
>> + */
>> + break;
>
> Copy and paste error... you meant groups here.
Yes, thanks. Fixed.
>
> *Patch #07 LDAP: Add sdap_get_and_parse_generic_send*
> ACK
>
> *Patch #08 LDAP: Use sdap_get_and_parse_generic_/_recv*
> ACK
>
> *Patch #09 LDAP: Add sdap_lookup_type enum*
> ACK
>
> *Patch #10 LDAP: Add the wildcard_limit option*
>
>> +ldap_pwdlockout_dn = str, None, false
>
> This probably belongs to separate patch...
Yeah, this was wrong.
>
> *Patch #11 IFP: Add wildcard requests*
>
>> size_t ifp_list_ctx_cap(struct ifp_list_ctx *list_ctx, size_t entries)
>
> ifp_list_ctx_remaining_capacity? I had no idea what the function does before
> I read its code.
OK, renamed.
Thanks for the review, new patches are attached.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel