[PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[Jeff Layton]

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.

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?

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 Makefile.am    | 12 ++++++--
 getcifsacl.c   | 68 +++++++++++++------------------------------
 idmap_plugin.c | 68 +++++++++++++++++++++++++++++++++++++++++++
 idmap_plugin.h | 40 ++++++++++++++++++++++++++
 idmapwb.c      | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 228 insertions(+), 51 deletions(-)
 create mode 100644 idmap_plugin.c
 create mode 100644 idmap_plugin.h
 create mode 100644 idmapwb.c

diff --git a/Makefile.am b/Makefile.am
index ff7a726..dab2957 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -56,9 +56,8 @@ endif
 
 if CONFIG_CIFSACL
 bin_PROGRAMS += getcifsacl
-getcifsacl_SOURCES = getcifsacl.c
-getcifsacl_LDADD = $(WBCLIENT_LIBS)
-getcifsacl_CFLAGS = $(WBCLIENT_CFLAGS)
+getcifsacl_SOURCES = getcifsacl.c idmap_plugin.c
+getcifsacl_LDADD = -ldl
 man_MANS += getcifsacl.1
 
 bin_PROGRAMS += setcifsacl
@@ -66,6 +65,13 @@ setcifsacl_SOURCES = setcifsacl.c
 setcifsacl_LDADD = $(WBCLIENT_LIBS)
 setcifsacl_CFLAGS = $(WBCLIENT_CFLAGS)
 man_MANS += setcifsacl.1
+
+plugindir = $(pkglibdir)
+plugin_PROGRAMS = idmapwb.so
+
+idmapwb.so: idmapwb.c
+	$(CC) $(CFLAGS) $(AM_CFLAGS) $(WBCLIENT_CFLAGS) $(LDFLAGS) -shared -fpic -o $@ $+ $(WBCLIENT_LIBS)
+
 endif
 
 SUBDIRS = contrib
diff --git a/getcifsacl.c b/getcifsacl.c
index 3f94a99..1768b16 100644
--- a/getcifsacl.c
+++ b/getcifsacl.c
@@ -34,10 +34,10 @@
 #include <stddef.h>
 #include <errno.h>
 #include <limits.h>
-#include <wbclient.h>
 #include <ctype.h>
 #include <sys/xattr.h>
 #include "cifsacl.h"
+#include "idmap_plugin.h"
 
 static const char *prog;
 
@@ -172,61 +172,33 @@ print_ace_type(uint8_t acetype, int raw)
 	}
 }
 
-/*
- * Winbind keeps wbcDomainSid fields in host-endian. Copy fields from the
- * csid to the wsid, while converting the subauthority fields from LE.
- */
 static void
-csid_to_wsid(struct wbcDomainSid *wsid, const struct cifs_sid *csid)
+print_sid(struct cifs_sid *csid, int raw)
 {
-	int i;
-	uint8_t num_subauth = (csid->num_subauth <= WBC_MAXSUBAUTHS) ?
-				csid->num_subauth : WBC_MAXSUBAUTHS;
-
-	wsid->sid_rev_num = csid->revision;
-	wsid->num_auths = num_subauth;
-	for (i = 0; i < NUM_AUTHS; i++)
-		wsid->id_auth[i] = csid->authority[i];
-	for (i = 0; i < num_subauth; i++)
-		wsid->sub_auths[i] = le32toh(csid->sub_auth[i]);
-}
-
-static void
-print_sid(struct cifs_sid *sidptr, int raw)
-{
-	int i;
-	wbcErr rc;
-	char *domain_name = NULL;
-	char *sidname = NULL;
-	enum wbcSidType sntype;
+	int i, rc;
+	char *name;
 	unsigned long long id_auth_val;
-	struct wbcDomainSid wsid;
-
-	csid_to_wsid(&wsid, sidptr);
 
 	if (raw)
 		goto print_sid_raw;
 
-	rc = wbcLookupSid(&wsid, &domain_name, &sidname, &sntype);
-	if (WBC_ERROR_IS_OK(rc)) {
-		printf("%s", domain_name);
-		if (strlen(domain_name))
-			printf("%c", '\\');
-		printf("%s", sidname);
-		wbcFreeMemory(domain_name);
-		wbcFreeMemory(sidname);
-		return;
-	}
+	rc = sid_to_str(csid, &name);
+	if (rc)
+		goto print_sid_raw;
+
+	printf("%s", name);
+	free(name);
+	return;
 
 print_sid_raw:
-	printf("S-%hhu", wsid.sid_rev_num);
+	printf("S-%hhu", csid->revision);
 
-	id_auth_val = (unsigned long long)wsid.id_auth[5];
-	id_auth_val += (unsigned long long)wsid.id_auth[4] << 8;
-	id_auth_val += (unsigned long long)wsid.id_auth[3] << 16;
-	id_auth_val += (unsigned long long)wsid.id_auth[2] << 24;
-	id_auth_val += (unsigned long long)wsid.id_auth[1] << 32;
-	id_auth_val += (unsigned long long)wsid.id_auth[0] << 48;
+	id_auth_val = (unsigned long long)csid->authority[5];
+	id_auth_val += (unsigned long long)csid->authority[4] << 8;
+	id_auth_val += (unsigned long long)csid->authority[3] << 16;
+	id_auth_val += (unsigned long long)csid->authority[2] << 24;
+	id_auth_val += (unsigned long long)csid->authority[1] << 32;
+	id_auth_val += (unsigned long long)csid->authority[0] << 48;
 
 	/*
 	 * MS-DTYP states that if the authority is >= 2^32, then it should be
@@ -237,8 +209,8 @@ print_sid_raw:
 	else
 		printf("-0x%llx", id_auth_val);
 
-	for (i = 0; i < wsid.num_auths; i++)
-		printf("-%u", wsid.sub_auths[i]);
+	for (i = 0; i < csid->num_subauth; i++)
+		printf("-%u", csid->sub_auth[i]);
 }
 
 static void
diff --git a/idmap_plugin.c b/idmap_plugin.c
new file mode 100644
index 0000000..672f465
--- /dev/null
+++ b/idmap_plugin.c
@@ -0,0 +1,68 @@
+/*
+ * ID Mapping Plugin interface for cifs-utils
+ * Copyright (C) 2012 Jeff Layton (jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <dlfcn.h>
+#include <errno.h>
+#include <stdint.h>
+
+#include "cifsacl.h"
+
+#define DEFAULT_PLUGIN_LOCATION "/etc/cifs-utils/idmap-plugin"
+
+const char *plugin_errmsg;
+static void *plugin;
+
+static void *
+resolve_symbol(const char *symbol_name)
+{
+	void *symbol;
+
+	if (!plugin) {
+		plugin = dlopen(DEFAULT_PLUGIN_LOCATION, RTLD_LAZY);
+		if (!plugin) {
+			plugin_errmsg = dlerror();
+			return plugin;
+		}
+	}
+
+	dlerror();
+	symbol = dlsym(plugin, symbol_name);
+	if (!symbol)
+		plugin_errmsg = dlerror();
+	return symbol;
+}
+
+void
+close_plugin(void)
+{
+	if (!plugin)
+		return;
+	dlclose(plugin);
+}
+
+int
+sid_to_str(const struct cifs_sid *sid, char **name)
+{
+	int (*entry)(const struct cifs_sid *, char **);
+
+	*(void **)(&entry) = resolve_symbol("idmap_sid_to_str");
+	if (!entry)
+		return -ENOSYS;
+
+	return (*entry)(sid, name);
+}
diff --git a/idmap_plugin.h b/idmap_plugin.h
new file mode 100644
index 0000000..8853815
--- /dev/null
+++ b/idmap_plugin.h
@@ -0,0 +1,40 @@
+/*
+ * ID Mapping Plugin interface for cifs-utils
+ * Copyright (C) 2012 Jeff Layton (jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "cifsacl.h"
+
+/*
+ * On error, plugin functions should set this pointer to a string description
+ * of the error. The string should not be freed by the caller.
+ */
+extern const char *plugin_errmsg;
+
+/*
+ * External API. Programs should call this to use the plugin functionality.
+ */
+
+/* Convert cifs_sid to a string. Caller must free *name on success */
+extern int sid_to_str(const struct cifs_sid *sid, char **name);
+
+/*
+ * Plugins should implement the following functions. All of them
+ * return 0 on success and non-zero on error.
+ *
+ * Convert cifs_sid to a string. Caller must free *name on success
+ * extern int (*idmap_sid_to_str)(const struct cifs_sid *, char **);
+ */
diff --git a/idmapwb.c b/idmapwb.c
new file mode 100644
index 0000000..a2072d2
--- /dev/null
+++ b/idmapwb.c
@@ -0,0 +1,91 @@
+/*
+ * Winbind ID Mapping Plugin
+ * Copyright (C) 2012 Jeff Layton (jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdint.h>
+#include <endian.h>
+#include <string.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <wbclient.h>
+
+#include "idmap_plugin.h"
+
+/*
+ * Winbind keeps wbcDomainSid fields in host-endian. Copy fields from the
+ * csid to the wsid, while converting the subauthority fields from LE.
+ */
+static void
+csid_to_wsid(struct wbcDomainSid *wsid, const struct cifs_sid *csid)
+{
+	int i;
+	uint8_t num_subauth = (csid->num_subauth <= WBC_MAXSUBAUTHS) ?
+				csid->num_subauth : WBC_MAXSUBAUTHS;
+
+	wsid->sid_rev_num = csid->revision;
+	wsid->num_auths = num_subauth;
+	for (i = 0; i < NUM_AUTHS; i++)
+		wsid->id_auth[i] = csid->authority[i];
+	for (i = 0; i < num_subauth; i++)
+		wsid->sub_auths[i] = le32toh(csid->sub_auth[i]);
+}
+
+int
+idmap_sid_to_str(const struct cifs_sid *csid, char **string)
+{
+	int rc;
+	wbcErr wbcrc;
+	char *domain = NULL;
+	char *name = NULL;
+	enum wbcSidType sntype;
+	struct wbcDomainSid wsid;
+	size_t len;
+
+	csid_to_wsid(&wsid, csid);
+
+	wbcrc = wbcLookupSid(&wsid, &domain, &name, &sntype);
+	if (!WBC_ERROR_IS_OK(wbcrc)) {
+		plugin_errmsg = wbcErrorString(wbcrc);
+		return -EIO;
+	}
+
+	/* +1 for '\\' and +1 for NULL terminator */
+	len = strlen(domain) + 1 + strlen(name) + 1;
+
+	*string = malloc(len);
+	if (!*string) {
+		plugin_errmsg = "Unable to allocate memory";
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = snprintf(*string, len, "%s\\%s", domain, name);
+	if (rc >= (long)len) {
+		free(*string);
+		plugin_errmsg = "Resulting string was truncated";
+		*string = NULL;
+		rc = -EIO;
+	} else {
+		rc = 0;
+	}
+out:
+	wbcFreeMemory(domain);
+	wbcFreeMemory(name);
+	return rc;
+}
-- 
1.7.11.7

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[simo]

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.

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

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(..);
}

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Principal Software Engineer at Red Hat, Inc. <simo@redhat.com>

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[Jeff Layton]

On Thu, 06 Dec 2012 08:42:31 -0500
simo <idra-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 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)?

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

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

> 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...
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[Scott Lovenberg]

On Thu, Dec 6, 2012 at 7:37 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> 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.

Please no symlinks.  We could end up with something like alternatives
(/etc/alternatives) where you need a utility to track and change
symlinks pointing to symlinks.  I don't even know where java is on my
machine or which JVM I'm running.  Symlinks have a tendency to turn
into symlink farms.  Look at your PAM install, it's a bunch of links
to a single file on most installs.  That way lies madness and dragons.

I think most people are comfortable with config files and they're
intuitive.  Also, is anyone at any time going to be using more than
one mapping interface?

-- 
Peace and Blessings,
-Scott.

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[simo]

On Thu, 2012-12-06 at 09:58 -0500, Jeff Layton wrote:
> On Thu, 06 Dec 2012 08:42:31 -0500
> simo <idra-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 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.

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

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

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

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Principal Software Engineer at Red Hat, Inc. <simo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[simo]

On Thu, 2012-12-06 at 10:16 -0500, Scott Lovenberg wrote:
> On Thu, Dec 6, 2012 at 7:37 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > 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.
> 
> Please no symlinks.  We could end up with something like alternatives
> (/etc/alternatives) where you need a utility to track and change
> symlinks pointing to symlinks.  I don't even know where java is on my
> machine or which JVM I'm running.  Symlinks have a tendency to turn
> into symlink farms.  Look at your PAM install, it's a bunch of links
> to a single file on most installs.  That way lies madness and dragons.
> 
> I think most people are comfortable with config files and they're
> intuitive.  Also, is anyone at any time going to be using more than
> one mapping interface?

Probably not, which is why a symlink is all we need.
I think starting with a symlink is fine, I do not particularly love it,
but makes a lot of stuff simpler for starter.
Having to parse a config file at each program invocation (for utilities)
adds up in latency and also requires YACF (Yet Another Config File)
where all you do is say: use plugin X, or a very complex version of a
symlink.

Now if you need to actually configure something then yes we could move
to use a config file eventually.

But unless you already know there is something the cifs utils (*not* the
plugins) need to configure then it seem a lot of overhead both in terms
of coding that needs to be done, file formats to choose and associated
bikeshed, and runtime churn.

Simo.


-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Principal Software Engineer at Red Hat, Inc. <simo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[Scott Lovenberg]

On Thu, Dec 6, 2012 at 10:38 AM, simo <idra-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, 2012-12-06 at 10:16 -0500, Scott Lovenberg wrote:
>> On Thu, Dec 6, 2012 at 7:37 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> > 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.
>>
>> Please no symlinks.  We could end up with something like alternatives
>> (/etc/alternatives) where you need a utility to track and change
>> symlinks pointing to symlinks.  I don't even know where java is on my
>> machine or which JVM I'm running.  Symlinks have a tendency to turn
>> into symlink farms.  Look at your PAM install, it's a bunch of links
>> to a single file on most installs.  That way lies madness and dragons.
>>
>> I think most people are comfortable with config files and they're
>> intuitive.  Also, is anyone at any time going to be using more than
>> one mapping interface?
>
> Probably not, which is why a symlink is all we need.
> I think starting with a symlink is fine, I do not particularly love it,
> but makes a lot of stuff simpler for starter.
> Having to parse a config file at each program invocation (for utilities)
> adds up in latency and also requires YACF (Yet Another Config File)
> where all you do is say: use plugin X, or a very complex version of a
> symlink.
>
> Now if you need to actually configure something then yes we could move
> to use a config file eventually.
>
> But unless you already know there is something the cifs utils (*not* the
> plugins) need to configure then it seem a lot of overhead both in terms
> of coding that needs to be done, file formats to choose and associated
> bikeshed, and runtime churn.
>
> Simo.

Simo, those are all great points.  FWIW, even though I don't like the
idea of symlinks, I'm inclined to agree with you that in this case
it's the correct way to go.

-- 
Peace and Blessings,
-Scott.

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[Jeff Layton]

On Thu, 06 Dec 2012 10:34:10 -0500
simo <idra@samba.org> wrote:

> On Thu, 2012-12-06 at 09:58 -0500, Jeff Layton wrote:
> > On Thu, 06 Dec 2012 08:42:31 -0500
> > simo <idra@samba.org> 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@samba.org>

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[Jeff Layton]

On Thu, 06 Dec 2012 08:42:31 -0500
simo <idra-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 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.
> 
> > 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.
> 

Now that I look, I'm not sure I understand the point of the above...

The function that the program calls is called "sid_to_str()". I already
added some "namespacification" around that by prefixing it with
"idmap_", so the function provided by the plugin is called
"idmap_sid_to_str()". What's the benefit of adding another prefix to
that?

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH RFC] cifs-utils: new plugin architecture for ID mapping code

[simo]

On Thu, 2012-12-06 at 16:12 -0500, Jeff Layton wrote:
> On Thu, 06 Dec 2012 08:42:31 -0500
> simo <idra@samba.org> 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.
> > 
> > > 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.
> > 
> 
> Now that I look, I'm not sure I understand the point of the above...
> 
> The function that the program calls is called "sid_to_str()". I already
> added some "namespacification" around that by prefixing it with
> "idmap_", so the function provided by the plugin is called
> "idmap_sid_to_str()". What's the benefit of adding another prefix to
> that?

idmap_ is quite generic and may be used inside samba for example so it
may not be easy to expose the symbol to you, but yeah I did not
understand you used idmap_ as namespace, so I guess my advice is to find
a better namespace more unique to cifs.ko :)

Simo.


-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Principal Software Engineer at Red Hat, Inc. <simo@redhat.com>