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.
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.
*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?
*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?
static bool cache_req_cachable(struct cache_req_input *input)
How about cache_req_avoid_cache?
*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?
*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.
*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...
*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.