> > 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!