> > > 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!
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
