Maybe I'm not seeing it, but if an unexpected error occurs anywhere during the setautomntent_send() processing, we never release the in-construction map entry in the cache. I think that's going to break subsequent lookups for that same map (it will never reply).
I'm sorry, I don't quite understand. I only saw a couple of cases where we wouldn't remove the map (state->map of struct autofs_map_ctx *) if an operation failed after we set the map into the hash. Is that what you meant?
Never mind, I was misreading the patch. I was thinking that it was possible for a request to the cache/dp to terminate with an error such that map->ready was never set to TRUE.
On further investigation, it looks to me like we're just setting map->found to FALSE if we get an error. Which is probably a pretty reasonable approach.
As previously-stated, I'd like to see us return larger chunks to the client at a time, so we're not going across the pipe for every entry. But this can be done post-beta.
Yes, that's being tracked.
In sss_autofs_cmd_getautomntent() you probably only want to be checking the UTF-8 status of the 'name', not the complete body. The namelen portion of the buffer could break this. Same for sss_autofs_cmd_getautomnbyname() for both name and key.
Fixed.
[PATCH 10/10] AUTOFS: LDAP provder The whole LDAP provider in a single patch. One place that might need improving is removing a map from an entry. The entry might become orphaned when no maps link to it and the cache might grow. I will improve the patch by searching for entries with no memberof: links and deleting them after the save.
There is neither enumerate task nor any periodic download. I'm not quite sure it is needed. I'll ask Ian about that.
I will review this tomorrow.
Nack.
Nitpick: please remove the tabs you added in +if BUILD_AUTOFS +libsss_ldap_la_SOURCES += src/providers/ldap/sdap_autofs.c \
src/providers/ldap/sdap_async_autofs.c
+endif
and
+if BUILD_AUTOFS +libsss_ipa_la_SOURCES += src/providers/ldap/sdap_autofs.c \
src/providers/ldap/sdap_async_autofs.c
+endif
Fixed
Spot the copy-paste error ;-) +#ifdef BUILD_AUTOFS +/* sudo */ +void sdap_autofs_handler(struct be_req *be_req); +#endif
It's actually not necessary to have the autofs handler in a header file, I completely removed the declaration.
sdap_autofs_setautomntent_send() doesn't handle a NULL mapname properly (which you expressly allow in sdap_autofs_handler()).
Done.
Don't store the filter-sanitized version of the mapname in struct sdap_autofs_setautomntent_state(). Keep the original version around and sanitize it for filters where appropriate. Otherwise, you may end up double-filtering if you pass it into functions that do this for you, like sysdb_get_map_byname() should be doing (see above).
Done.
In sdap_get_automntmap_process() you throw an error if a search returns more than one map. I assume this is because we're not supporting enumeration, but it's inconsistent with allowing NULL/<ALL> in sdap_autofs_setautomntent_send(). Please pick one and make it consistent.
Fixed by disallowing NULL names in sdap_autofs_setautomntent_send(). We can extend the whole logic to handle enumerations should it turn out that autofs client needs it.
automntmaps_process_members_send() should test whether the requested entry is present in ANY of the search bases (not just the current one). This can be fixed post-beta, please open a ticket.
https://fedorahosted.org/sssd/ticket/1168
I think the comment in sdap_autofs_setautomntent_save() when receiving an error from sysdb_get_map_byname() is wrong and misleading.
Done.
Otherwise, I think the logic looks good. Great job!
Attached is a tarball that contains one more change in the configAPI.
Ack to all patches and pushed to master. Great work!