On Thu, 06 Dec 2012 10:34:10 -0500 simo <idra@xxxxxxxxx> wrote: > On Thu, 2012-12-06 at 09:58 -0500, Jeff Layton wrote: > > On Thu, 06 Dec 2012 08:42:31 -0500 > > simo <idra@xxxxxxxxx> wrote: > > > > > On Thu, 2012-12-06 at 07:37 -0500, Jeff Layton wrote: > > > > Currently, the ACL-related tools in cifs-utils call into the wbclient > > > > libs directly in order to do their bidding. The wbclient developers want > > > > to get away from needing to configure winbind on the clients and instead > > > > allow sssd to handle the id-mapping. Less muss, less fuss... > > > > > > > > This patch represents an initial step in that direction. It adds a > > > > plugin architecture for cifs-utils, adds wrappers around the calls into > > > > libwbclient that find an idmap plugin library to use and then has it > > > > call into that plugin to do the actual ID mapping. > > > > > > > > This patch should be considered an RFC on the overall design. Once I > > > > have some positive feedback (or lack of negative feedback), I'll do the > > > > same with cifs.idmap and setcifsacl. > > > > > > > > This patch is still pretty rough, but should demonstrate the basic > > > > design: > > > > > > > > The application will call into a set of routines that find the correct > > > > plugin and dlopen() it. Currently the plugin is located in a hardcoded > > > > location that will eventually be settable via autoconf. That location is > > > > intended to be a symlink that points to the real plugin (generally under > > > > %libdir/cifs-utils). > > > > > > > > The plugin will export a number of functions with well-known names. The > > > > wrappers find those by using dlsym() and then call them. > > > > > > > > I'm tracking progress on this work here: > > > > > > > > https://bugzilla.samba.org/show_bug.cgi?id=9203 > > > > > > > > Here are some questions to ponder: > > > > > > > > 1/ Should we switch this code to use a config file of some sort instead > > > > of this symlink? The symlink would probably be more efficient, but it > > > > is a little odd and might confuse people. It also might make it hard to > > > > expand the idmapping interfaces later. > > > > > > I think a symlink is ok for starters, a config file can always be added > > > later if needed. > > > > > > > To play devil's advocate, a config file might make more sense. What if > > a plugin wants to be able to set certain parameters globally (domain > > names or something)? > > Then the plugin will have it's own file. > > Which made me understand what looked strange in your code. You are not > calling an initialization function which is customary to do, so plugins > can do their setup. > Of course plugins can also do lazy init the first time you call any of > their function, but calling a init plugin explicitly may be useful, esp > if you pass in a 'interface version number', so that should you require > changes to the interface in future the plugin may adapt and you can use > the same with multiple cifs versions. > Ok, another good point. I'll put that in. > > Having that configuration in a single place might be less confusing > > than having to set a symlink and set up a config file. Switching to a > > config file later is a UI change and those are always more painful... > > If you defer configuration to the plugin it is not your problem, and I > think that would be the correct way to go, otherwise you need to provide > methods for the plugins to read this config file and it quickly winds > down becoming a complex and more tightly coupled interface. > > I think you want to stay out of plugins configuration business, they can > take care of that on their own. > > If you want to make things easier maybe call an initializer function and > expect an opaque handle out of it. > Then always pass that handle back in any plugin function. This way the > interface can also be thread safe w/o forcing the plugin to use mutexes, > should you ever need thread safety. > Ok, that sounds reasonable and simple, I'll add that in too. > > > > 2/ Should I switch this code to use libltdl for the plugin architecture? > > > > I started to use that initially, but it was awfully complex to get working. > > > > Since portability isn't really a concern with cifs-utils, I punted. Does > > > > anyone see issues with rolling our own here? > > > > > > Well the cifs kernel module is Linux only, I would leave the hassle of > > > dealing with portability to whomever would like to port this. > > > Using dlopen/dlsym is simple, so roll-your-own seem fine to me. > > > > > > One thing though I would name-space the symbol, so instead of > > > idmap_sid_to_str call it cifs_idmap_sid_to_str, in the plugin. > > > Internally you can call whatever you want. > > > > > > > Ok, the namespace thing is probably a good idea. Perhaps we should even > > take a page out of the libltdl book, and prefix the symbols with the > > name of the plugin? So in this patch, that would be something like > > "idmapwb_sid_to_str". That way if we ever want to be able to stack > > plugins, we can potentially do so without conflicts. > > It is not really needed, because you are not going to load the symbols > as is, but you assign them to an internal name. > And I do not think you are ever going to support multiple idmappers, but > even if you do by using dlsym() you shouldn't really care about names, > because you have handle to a specific shared object and you can assign > and rename those symbols when you resolve them (as i showed in my > example) so you still do not get a conflict issue). > > Namespacing using plugin names is needed if you want to just dlopen() > and then use the plugin's names directly without an explicit dlsym(). > In that case name conflicts would arise. > Yeah, that occurred to me after I sent this. Probably no need to add that complexity since we're going to rely on dlsym(). > > > Also I think you shouldn't resolve symbols each time, > > > > > > Declare a function pointer in the data segment (so inited to NULL at > > > startup) and do a lazy init only if it is NULL, by assigning it. > > > > > > #define RESOLVE_SYMBOL(name) \ > > > do { \ > > > if (name == NULL) { \ > > > name = resolve_symbol("cifs_" # name); \ > > > if (name == NULL) \ > > > return -ENOSYS; \ > > > } \ > > > } while(0); > > > > > > sid_to_str() > > > { > > > RESOLVE_SYMBOL(idmap_sid_to_str); > > > return idmap_sid_to_str(..); > > > } > > > > > > > Yep, I was planning to add that in a later patch. I just wanted to make > > the initial patch simple to focus on the interfaces between components. > > > > Thanks for the review so far... > > YW, > HTH. > > Simo. > Thanks again, -- Jeff Layton <jlayton@xxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
