On Fri, 2012-02-03 at 16:46 +0100, Jakub Hrozek wrote: > Hi, > > attached is a series of patches that implements autofs support. In order > to test it, you can either use a provided command-line client or patch > autofs and test directly using automounter. > > I have patched the rawhide version of automounter and put it into my > fedorapeople account until it lands in Fedora: > http://jhrozek.fedorapeople.org/autofs-sssd/ > > Alternatively, you can apply the patches directly onto autofs-5.0.6: > http://jhrozek.fedorapeople.org/autofs-sssd/autofs-5.0.6-sss-lookup-module.patch > http://jhrozek.fedorapeople.org/autofs-sssd/autofs-5.0.6-teach-automount-about-sss-source.patch > > The patches are functionally complete with the single exception of the > IPA provider. I will finish it during today. To test with an IPA server, > you need to set "autofs_provider = ldap" manually. > > There is a bug in the responder where we don't properly clean up after > setent, but I don't think I should hold off sending the patches for review. > The responder also uses the deprecated cache checking logic (see patch > 0008). I will either finish that before 1.7.9 or during 1.8 development. > The responder also does not perform an implicit setautomntent() yet. A > fix for none of these issues would need a string freeze break. > > The patches are based on the sudo patches on-list and Stephen's pending > cache timeout patch and must be applied on top of them. > > [PATCH 01/10] BUILD: Introduce a --with-autofs config option > This would allow to select the autofs feature during build without > having to select the other features. >
Ack > [PATCH 02/10] SYSDB: Remove code duplication between member_add and member_del > Refactoring patch that will be used in the next one. > Ack > [PATCH 03/10] AUTOFS: sysdb interface > The cache interface. > Nack (minor). sysdb_autofsmap_strdn() should be static. It's not used outside of sysdb_autofs.c > [PATCH 04/10] AUTOFS: a client library > This is the library the autofs client is using. automounter dlopen()s > the library so there is no header file, no pkgconfig file and the > library is in the libsss_autofs package, not in -devel. > > The library provides the following interface that translates into > responder interface in the next patch: > * _sss_setautomntent() - select the map for processing > * _sss_getautomntent_r() - iterates through key/value pairs in the > selected map. The key is usually the mount > point, the value is mount information > (server:/export) > * _sss_getautomntbyname_r() - returns value for a specific key. > * _sss_endautomntent() deselect a map, clean up > > As you can see, the interface closely resembles netgroup interface glibc > uses, so I was able to reuse many concepts from the netgroup code. > Nack. The autofs plugin library should not be versioned. Autofs isn't linking against it, so there's no benefit to including version information. In the case of plugins, the interface is defined by the application calling dlopen(), not the library. This is how we handle our libsss_*.so libraries too, as well as the krb5 locator plugin (if you need an example of a library we're exposing to an external application) The .exports file has indentation issues and also includes some tabs instead of spaces. While it's probably too late now to change the public interface (since the autofs folks are working against this), I do wish we'd stuck with our policy of function arguments being inputs followed by outputs. I don't like how the "context" argument is last (except in _sss_setautomntent(), of course). Potential memory leak in _sss_setautomntent(): if the strdup(name) fails, you're not freeing "name" before exiting. Use the safealign_memcpy() from util.h for copying the mapname into the request buffer in _sss_getautomntent_r() (mostly for consistency. It's identical code to what you wrote). General architecture question: Why don't we just store the result data in the "struct automtent" when we complete the _sss_setautomntent() routine? Granted, we'd be carrying around more memory in autofs, but it would be accessible quite a bit faster than going back and forth to the responder for multiple _sss_getautomntbyname_r() calls. At the very minimum, it would be better to replicate what we're doing in the NSS passwd client, where we get SSS_NSS_MAX_ENTRIES at a time. I can see this being a bottleneck for a system looking up user home directories for thousands of users. That said, this would be a performance enhancement and I'd be fine with modifying this after feature freeze. (The public interface will remain the same). > [PATCH 05/10] AUTOFS: a command-line test client > A very simply binary that can be used to test getting data from the > library via SSSD in pretty much the same way SSSD would. A required > positional parameter specifies the map name and the tool would print out > all the key/value pairs using _sss_getautomntent_r(). You can also > specify -n to query a specific key using _sss_getautomntbyname_r(). > > There is one gotcha in the code - the library location is hardcoded to > ".libs". Is there a way to get this from libtool? Is there a really good reason not to just create a private header for the exposed functions and just link directly against it? I think that's perfectly acceptable for a test tool. Sure, you're not testing that it dlopen()s properly, but that's pretty safe to assume that if it links it can be dlopen()ed. Otherwise, Ack. (And no, I don't know off-hand any way to get libtool to report the default output location of libraries) > > [PATCH 06/10] AUTOFS: Data Provider request > Implements the request that the responder might use to communicate with > the back end. > Nack. Please use the new DEBUG level macros. Functionally, the DP request looks good. > [PATCH 07/10] Refactor setent_req_list > Makes the setent_add_ref() and setent_notify_*() functions more generic > to be reusable by the autofs responder. > Ack > [PATCH 08/10] Split the logic to check cache expiration into separate function > The autofs responder can't reuse the check_cache function, so I split > the cache expiration check logic into a separate function instead. > As mentioned earlier, this patch might not be needed if I can finish the > rewrite to the new cache check logic. > Ack > [PATCH 09/10] AUTOFS: responder > The responder process. The patch seems big, but many parts are just > generic responder plumbing. > I will review this tomorrow. > [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.
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
