Re: tabs and formating - was Re: [PATCH] winbind id assignment module
>> There does not seem to be a style book for coding on the SAMBA project. > >Thank goodness... There are some guidelines, more about technical content than format, in the HEAD cvs tree, in samba/source/CodingSuggestions Jim McDonough IBM Linux Technology Center Samba Team 6 Minuteman Drive Scarborough, ME 04074 USA [EMAIL PROTECTED] [EMAIL PROTECTED] Phone: (207) 885-5565 IBM tie-line: 776-9984
Re: tabs and formating - was Re: [PATCH] winbind id assignment module
On Fri, May 24, 2002 at 11:53:53AM -0500, John E. Malmberg wrote: : > There does not seem to be a style book for coding on the SAMBA project. Thank goodness... Samba style rule-of-thumb: When editing a file, always keep the existing format. If there are multiple formats in the file, do only *minor* fixes (people like to look at diffs, after all). If rewriting wholesale...then make sure it's readable by others. Chris -)- -- Samba Team -- http://www.samba.org/ -)- Christopher R. Hertel jCIFS Team -- http://jcifs.samba.org/ -)- ubiqx development, uninq. ubiqx Team -- http://www.ubiqx.org/ -)- [EMAIL PROTECTED] OnLineBook -- http://ubiqx.org/cifs/-)- [EMAIL PROTECTED]
tabs and formating - was Re: [PATCH] winbind id assignment module
Mike Gerdts wrote: > On Sat, 2002-05-18 at 20:54, Andrew Bartlett wrote: > >>This looks *much* better. >> >>I'm not sure on the 'reload' functionality, but I suppose its a good >>idea. Other than minor things like indenting, (Try 8-space tabs) I >>think this is well on its way to inclusion. > > As for spacing... I tried to follow the standard that I saw in the file > already. I used 4 character tabs, but they should have expanded out > OK. I thought that others were using 4 character tabs as well because > sections of winbindd_idmap.c have tabs expanded to four characters. In > any case, the next version that goes out will get rid of any expanded > tabs and any necessary reformatting for prettiness will take place. Almost all terminal emulators and printing devices assume that the tab character stops on 8 character boundaries. Some editors will locally override it. So be aware that if you have set your tab stops to 4 characters, it will probably show up as 8 characters on many systems. Using all spaces instead of tabs solves that problem, but it really is convenient to have tabs there when using the arrow keys to navigate in a file. There does not seem to be a style book for coding on the SAMBA project. -John [EMAIL PROTECTED] Personal Opinion Only
Re: [PATCH] winbind id assignment module
Mike Gerdts wrote: > winbindd: Fixed up formatting to get rid of 4-space tabs that existed > before. Builds off of today's SAMBA_2_2 branch. While they are pretty similar ATM, I need patches to be against and tested with HEAD. I don't apply stuff to 2.2. Other than that, I have some other comments: It would be worthwile setting up soem state for the plugin, where it can store things like connection pointers etc, talloc etc. Make the init function's return include a void * that you always pass back to it. Also add a 'shutdown' function so that the plugin can clean up before we shutdown winbind or we reload the plugin. How does that sound? Andrew Bartlett -- Andrew Bartlett [EMAIL PROTECTED] Manager, Authentication Subsystems, Samba Team [EMAIL PROTECTED] Student Network Administrator, Hawker College [EMAIL PROTECTED] http://samba.org http://build.samba.org http://hawkerc.net
Re: [PATCH] winbind id assignment module
On Sat, 2002-05-18 at 20:54, Andrew Bartlett wrote:
> > The patch and a sample module are attached. The tarball also includes a
> > readme, changelog, and a Makefile.
>
> This looks *much* better.
>
> I'm not sure on the 'reload' functionality, but I suppose its a good
> idea. Other than minor things like indenting, (Try 8-space tabs) I
> think this is well on its way to inclusion.
A new rev of the patch is out. Here is the changelog for this release.
winbindd: Fixed up formatting to get rid of 4-space tabs that existed
before. Builds off of today's SAMBA_2_2 branch.
winbindd: If idmap object is defined but fails to load, it no longer
reverts to sequential assignment
The patch, a sample module, changelog, and a readme are available at
http://www.cae.wisc.edu/~gerdts/samba/idmap_file-0.0.4.tar.gz
Enjoy!
Mike
Index: source/nsswitch/winbindd.c
===
RCS file: /cvsroot/samba/source/nsswitch/winbindd.c,v
retrieving revision 1.3.2.29
diff -u -r1.3.2.29 winbindd.c
--- source/nsswitch/winbindd.c 8 May 2002 23:33:31 - 1.3.2.29
+++ source/nsswitch/winbindd.c 20 May 2002 17:23:17 -
@@ -66,6 +66,7 @@
}
load_interfaces();
+ load_idmap();
return(ret);
}
Index: source/nsswitch/winbindd.h
===
RCS file: /cvsroot/samba/source/nsswitch/winbindd.h,v
retrieving revision 1.3.4.8
diff -u -r1.3.4.8 winbindd.h
--- source/nsswitch/winbindd.h 10 Apr 2002 00:40:10 - 1.3.4.8
+++ source/nsswitch/winbindd.h 20 May 2002 17:23:17 -
@@ -203,4 +203,15 @@
#define SETENV(name, value, overwrite) ;
#endif
+/* Required for the winbindd UID/GID mapping plugin */
+
+#define WINBINDD_IDMAP_INTERFACE_VERSION 0
+extern struct winbind_idmap_ops *idmap_ops;
+
+/* Functions for winbind plug-ins */
+
+struct winbind_idmap_ops {
+ BOOL (*allocate_id)(DOM_SID *sid, uid_t *id, BOOL isgroup);
+};
+
#endif /* _WINBINDD_H */
Index: source/nsswitch/winbindd_idmap.c
===
RCS file: /cvsroot/samba/source/nsswitch/winbindd_idmap.c,v
retrieving revision 1.3.4.13
diff -u -r1.3.4.13 winbindd_idmap.c
--- source/nsswitch/winbindd_idmap.c 27 Apr 2002 03:04:08 - 1.3.4.13
+++ source/nsswitch/winbindd_idmap.c 20 May 2002 17:23:17 -
@@ -34,11 +34,91 @@
static TDB_CONTEXT *idmap_tdb;
+struct winbind_idmap_ops *idmap_ops; /* idmap plug-in */
+
+/* (Re)load the id allocation plugin */
+
+BOOL load_idmap(void) {
+ BOOL rv;
+ struct winbind_idmap_ops* (*idmap_init)(int *);
+ static void *idmap_object = NULL;
+ char *libfile;
+ int idmap_version;
+
+ libfile = lp_winbind_idmap_object();
+
+ /* Disable any previously loaded idmap object */
+ if ( *libfile == '\0' ) {
+ DEBUG(5, ("No winbindd idmap object defined\n"));
+ rv = True;
+ goto bail;
+ }
+
+ /* if it was previously loaded, unload it before reloading */
+ /* TODO: determine if this is even a good thing to support */
+ if ( idmap_object != NULL ) {
+ sys_dlclose(idmap_object);
+ }
+
+ idmap_object = sys_dlopen(libfile, RTLD_NOW | RTLD_GLOBAL);
+ if ( idmap_object == NULL ) {
+ DEBUG(0, ("Error opening '%s': %s\n", libfile, sys_dlerror()));
+ rv = False;
+ goto bail;
+ }
+
+ idmap_init = sys_dlsym(idmap_object, "idmap_init");
+ if ( idmap_init == NULL ) {
+ DEBUG(0, ("No idmap_init() symbol found in %s\n", libfile));
+ rv = False;
+ goto bail;
+ }
+
+ if ( (idmap_ops = idmap_init(&idmap_version)) == NULL ) {
+ DEBUG(0, ("idmap_init function from %s failed\n", libfile));
+ rv = False;
+ goto bail;
+ }
+
+ if ( idmap_version != WINBINDD_IDMAP_INTERFACE_VERSION ) {
+ DEBUG(0, ("idmap_init returned wrong interface version info (was %d, should be %d)\n",
+ idmap_version, WINBINDD_IDMAP_INTERFACE_VERSION));
+ rv = False;
+ goto bail;
+ }
+
+ DEBUG(5, ("Loaded winbind idmap object '%s'\n", libfile));
+ DEBUG(5, ("idmap_ops->allocate_id is %sdefined\n",
+idmap_ops->allocate_id ? "" : "NOT "));
+ return True;
+
+bail:
+ if ( idmap_object ) {
+ sys_dlclose(idmap_object);
+ idmap_object = NULL;
+ }
+ idmap_ops = NULL;
+ return rv;
+}
+
/* Allocate either a user or group id from the pool */
-static BOOL allocate_id(uid_t *id, BOOL isgroup)
+static BOOL allocate_id(DOM_SID *sid, uid_t *id, BOOL isgroup)
{
int hwm;
+ char *idmapfile;
+
+ if ( idmap_ops && idmap_ops->allocate_id ) {
+ DEBUG(4,("allocate_id using module '%s'\n",
+ lp_winbind_idmap_object()));
+ return(idmap_ops->allocate_id(sid, id, isgroup));
+ }
+
+ if ( *(lp_winbind_idmap_object()) ) {
+ DEBUG(0,("allocate_id configured to use idmap module, but "
+"module failed to load\n"));
+ return(False);
+ }
/* Get current high water mark */
@@ -105,7 +185,7 @@
/* Allocate a new id for this sid */
-if (id && allocate_id(id, isgroup)) {
+if (id && allocate_id(sid, id, isgroup)) {
fstring keystr2;
Re: Attachments - Re: [PATCH] winbind id assignment module
"John E. Malmberg" wrote: > > Andrew Bartlett wrote: > > Mike Gerdts wrote: > > > >>I am confused on list etiquette... I thought that samba-technical was > >>one that everyone complained about attachments 'cause it screwed up the > >>-digest version. I think that was just 'samba' BTW. This list has always taken all this stuff. > > They complain about binary attachments, and html mail. Patches havn't > > caused an issue as far as I know. Works best if you can convince your > > mailer to attach it (as yours did) in a 'text/...' format, as then it > > doesn't get encoded etc. > > Mozilla .99 is not handling the last attachment well, but that is it's > problem. It is continuously trying to reformat it to fit the preview > window. > > One of the problem with attachments is that many e-mail clients can not > control what format the attachment is encoded with. > > IMHO, it is better to post inline, and send an attachment as a personal > E-mail on request. The problem with inline is that some e-mail programs eat it... > Isn't there a separate mailing list for patches? Not a mailing list, but a jitterbug setup. As I'm not attached to the automatic e-mail setup on it, I find it a pain to deal with - its just *much* easier to do stuff on the list, particularly with small patches. Larger patches should be a link, possibly to the samba-patches URL. > In that case, it is better to discuss the patch and it's implications > here, but send the patch to the appropriate patch mailing list. Users > of that list are probably more likely to use a mail client that deals > with attachments. The problem is, the patch and the discussion need to be in the same place. Sending the patch (unless very large) to another place just makes it harder for me to process it. I refer here to the start of this thread, where I said 'that looks ok' to the general concepts, because the patch was a little difficult to find (inside a tarball, on a website). It turned out, when I finally looked at the patch that it needed some serious work. Thats why the two need to stay together. The main use for samba-patches is to keep track of patches over time (so they don't drop to the bottom of the inbox etc). The problem with that is that people have to be reading samba-patches... > Is there a FAQ entry to point people at? I'm not sure, but what we do have hasn't been maintained in *years*. Andrew Bartlett -- Andrew Bartlett [EMAIL PROTECTED] Manager, Authentication Subsystems, Samba Team [EMAIL PROTECTED] Student Network Administrator, Hawker College [EMAIL PROTECTED] http://samba.org http://build.samba.org http://hawkerc.net
Re: [PATCH] winbind id assignment module
On Sat, 2002-05-18 at 20:54, Andrew Bartlett wrote: > This looks *much* better. > > I'm not sure on the 'reload' functionality, but I suppose its a good > idea. Other than minor things like indenting, (Try 8-space tabs) I > think this is well on its way to inclusion. The reload functionality was initially intended to be only load(). My initial look at the existing code suggested that it would get called again after getting a HUP (I have to look into that though...). To me it looked as though it would be easiest just to make it be able to handle a reload. I think that it should work as it is, but I haven't tested it. The two things that I would want to test are 1) does it do what you expect, and 2) does it free up all resources related to the file such that someone debugging a module can count on a day of HUPs rather than restarts does end up with 50 copies still mapped. As for spacing... I tried to follow the standard that I saw in the file already. I used 4 character tabs, but they should have expanded out OK. I thought that others were using 4 character tabs as well because sections of winbindd_idmap.c have tabs expanded to four characters. In any case, the next version that goes out will get rid of any expanded tabs and any necessary reformatting for prettiness will take place. > I like the checking of the .so at loadparm time - its a nice touch. Thanks! Mike
Attachments - Re: [PATCH] winbind id assignment module
Andrew Bartlett wrote: > Mike Gerdts wrote: > >>I am confused on list etiquette... I thought that samba-technical was >>one that everyone complained about attachments 'cause it screwed up the >>-digest version. > > They complain about binary attachments, and html mail. Patches havn't > caused an issue as far as I know. Works best if you can convince your > mailer to attach it (as yours did) in a 'text/...' format, as then it > doesn't get encoded etc. Mozilla .99 is not handling the last attachment well, but that is it's problem. It is continuously trying to reformat it to fit the preview window. One of the problem with attachments is that many e-mail clients can not control what format the attachment is encoded with. IMHO, it is better to post inline, and send an attachment as a personal E-mail on request. Isn't there a separate mailing list for patches? In that case, it is better to discuss the patch and it's implications here, but send the patch to the appropriate patch mailing list. Users of that list are probably more likely to use a mail client that deals with attachments. Is there a FAQ entry to point people at? -John [EMAIL PROTECTED] Personal Opinion Only
Re: [PATCH] winbind id assignment module
Mike Gerdts wrote: > > On Sat, 2002-05-18 at 20:05, Andrew Bartlett wrote: > > I don't think I actually looked at the code for the first patch, only > > the comments. Unless a patch is *really* big (in which case you should > > consider why this is the case) you would do well to always include the > > patch an either an inline or attached diff, not as a tarball. > > I am confused on list etiquette... I thought that samba-technical was > one that everyone complained about attachments 'cause it screwed up the > -digest version. They complain about binary attachments, and html mail. Patches havn't caused an issue as far as I know. Works best if you can convince your mailer to attach it (as yours did) in a 'text/...' format, as then it doesn't get encoded etc. > The patch and a sample module are attached. The tarball also includes a > readme, changelog, and a Makefile. This looks *much* better. I'm not sure on the 'reload' functionality, but I suppose its a good idea. Other than minor things like indenting, (Try 8-space tabs) I think this is well on its way to inclusion. I like the checking of the .so at loadparm time - its a nice touch. Andrew Bartlett -- Andrew Bartlett [EMAIL PROTECTED] Manager, Authentication Subsystems, Samba Team [EMAIL PROTECTED] Student Network Administrator, Hawker College [EMAIL PROTECTED] http://samba.org http://build.samba.org http://hawkerc.net
Re: [PATCH] winbind id assignment module
On Sat, 2002-05-18 at 20:05, Andrew Bartlett wrote:
> I don't think I actually looked at the code for the first patch, only
> the comments. Unless a patch is *really* big (in which case you should
> consider why this is the case) you would do well to always include the
> patch an either an inline or attached diff, not as a tarball.
I am confused on list etiquette... I thought that samba-technical was
one that everyone complained about attachments 'cause it screwed up the
-digest version.
The patch and a sample module are attached. The tarball also includes a
readme, changelog, and a Makefile.
Mike
Index: nsswitch/winbindd.c
===
RCS file: /cvsroot/samba/source/nsswitch/winbindd.c,v
retrieving revision 1.3.2.29
diff -u -r1.3.2.29 winbindd.c
--- nsswitch/winbindd.c 8 May 2002 23:33:31 - 1.3.2.29
+++ nsswitch/winbindd.c 18 May 2002 19:57:39 -
@@ -66,6 +66,7 @@
}
load_interfaces();
+ load_idmap();
return(ret);
}
Index: nsswitch/winbindd.h
===
RCS file: /cvsroot/samba/source/nsswitch/winbindd.h,v
retrieving revision 1.3.4.8
diff -u -r1.3.4.8 winbindd.h
--- nsswitch/winbindd.h 10 Apr 2002 00:40:10 - 1.3.4.8
+++ nsswitch/winbindd.h 18 May 2002 19:57:39 -
@@ -203,4 +203,16 @@
#define SETENV(name, value, overwrite) ;
#endif
+/* Required for the winbindd UID/GID mapping plugin */
+
+#define WINBINDD_IDMAP_INTERFACE_VERSION 0
+extern struct winbind_idmap_ops *idmap_ops;
+
+/* Functions for winbind plug-ins */
+
+struct winbind_idmap_ops {
+/* BOOL (*allocate_id)(DOM_SID *sid, uid_t *id, BOOL isgroup); */
+BOOL (*allocate_id)(DOM_SID *sid, uid_t *id, BOOL isgroup);
+};
+
#endif /* _WINBINDD_H */
Index: nsswitch/winbindd_idmap.c
===
RCS file: /cvsroot/samba/source/nsswitch/winbindd_idmap.c,v
retrieving revision 1.3.4.13
diff -u -r1.3.4.13 winbindd_idmap.c
--- nsswitch/winbindd_idmap.c 27 Apr 2002 03:04:08 - 1.3.4.13
+++ nsswitch/winbindd_idmap.c 18 May 2002 19:57:39 -
@@ -34,12 +34,84 @@
static TDB_CONTEXT *idmap_tdb;
+struct winbind_idmap_ops *idmap_ops;/* idmap plug-in */
+
+/* (Re)load the id allocation plugin */
+
+BOOL load_idmap(void) {
+BOOL rv;
+struct winbind_idmap_ops* (*idmap_init)(int *);
+static void *idmap_object = NULL;
+char *libfile;
+int idmap_version;
+
+libfile = lp_winbind_idmap_object();
+
+/* Disable any previously loaded idmap object */
+if ( *libfile == '\0' ) {
+DEBUG(5, ("No winbindd idmap object defined\n"));
+rv = True;
+goto bail;
+}
+
+ /* if it was previously loaded, unload it before reloading */
+ /* TODO: determine if this is even a good thing to support */
+ if ( idmap_object != NULL ) {
+sys_dlclose(idmap_object);
+ }
+
+idmap_object = sys_dlopen(libfile, RTLD_NOW | RTLD_GLOBAL);
+if ( idmap_object == NULL ) {
+DEBUG(0, ("Error opening '%s': %s\n", libfile, sys_dlerror()));
+rv = False;
+goto bail;
+}
+
+idmap_init = sys_dlsym(idmap_object, "idmap_init");
+if ( idmap_init == NULL ) {
+DEBUG(0, ("No idmap_init() symbol found in %s\n", libfile));
+rv = False;
+goto bail;
+}
+
+if ( (idmap_ops = idmap_init(&idmap_version)) == NULL ) {
+DEBUG(0, ("idmap_init function from %s failed\n", libfile));
+rv = False;
+goto bail;
+}
+
+if ( idmap_version != WINBINDD_IDMAP_INTERFACE_VERSION ) {
+DEBUG(0, ("idmap_init returned wrong interface version info (was %d, should be %d)\n",
+idmap_version, WINBINDD_IDMAP_INTERFACE_VERSION));
+rv = False;
+goto bail;
+}
+
+DEBUG(5, ("Loaded winbind idmap object '%s'\n", libfile));
+DEBUG(5, ("idmap_ops->allocate_id is %sdefined\n",
+idmap_ops->allocate_id ? "" : "NOT "));
+return True;
+
+bail:
+if ( idmap_object ) {
+sys_dlclose(idmap_object);
+idmap_object = NULL;
+}
+idmap_ops = NULL;
+return rv;
+}
+
/* Allocate either a user or group id from the pool */
-static BOOL allocate_id(uid_t *id, BOOL isgroup)
+static BOOL allocate_id(DOM_SID *sid, uid_t *id, BOOL isgroup)
{
int hwm;
+if ( idmap_ops && idmap_ops->allocate_id ) {
+DEBUG(4,("allocate_id using module '%s'\n", lp_winbind_idmap_object()));
+return(idmap_ops->allocate_id(sid, id, isgroup));
+}
+
/* Get current high water mark */
if ((hwm = tdb_fetch_int32(idmap_tdb,
@@ -105,7 +177,7 @@
/* Allocate a new id for this sid */
-if (id && allocate_id(id, isgroup)) {
+if (id && allocate_id(sid, id, isgroup)) {
fstring keystr2;
/* Store new id */
Index: param/loadparm.c
===
RCS file: /cvsroot/samba/source/pa
Re: [PATCH] winbind id assignment module
Mike Gerdts wrote: > http://www.cae.wisc.edu/~gerdts/samba/idmap_file-0.0.3.tar.gz > > Please don't sugar coat the comments on this rev of the patch. It makes > it so hard to figure out what people really think of it. :) I don't think I actually looked at the code for the first patch, only the comments. Unless a patch is *really* big (in which case you should consider why this is the case) you would do well to always include the patch an either an inline or attached diff, not as a tarball. This lowers the 'cost' of reviewing it, becouse I can just read it easily - particuarly when I'm just using mutt on a remote server etc. I'll try to get time to have a look a this, and get back to you. Andrew Bartlett -- Andrew Bartlett [EMAIL PROTECTED] Manager, Authentication Subsystems, Samba Team [EMAIL PROTECTED] Student Network Administrator, Hawker College [EMAIL PROTECTED] http://samba.org http://build.samba.org http://hawkerc.net
Re: [PATCH] winbind id assignment module
On Sat, 2002-05-18 at 11:04, Andrew Bartlett wrote: > Mike Gerdts wrote: > > > > I have created a nearly new implementation of the winbindd uid/gid > > assignment plugin that I first released about a week ago[1]. You can > > get the latest patch and sample module at > > http://www.cae.wisc.edu/~gerdts/samba/. > > I'm not sure how closely I looked at the last one, but this looks > HORRID! After removing the emphasis that was put on the cost of a socket call, the patch should be a lot less horrid now. It no longer touches smbd. With this rev you will need to be sure that your winbind [ug]id parameters include the full range of id's that you may want to allocate, even if you have lots of non-winbind id's in that range. http://www.cae.wisc.edu/~gerdts/samba/idmap_file-0.0.3.tar.gz Please don't sugar coat the comments on this rev of the patch. It makes it so hard to figure out what people really think of it. :) Mike
Re: [PATCH] winbind id assignment module
Mike Gerdts wrote: > > I have created a nearly new implementation of the winbindd uid/gid > assignment plugin that I first released about a week ago[1]. You can > get the latest patch and sample module at > http://www.cae.wisc.edu/~gerdts/samba/. I'm not sure how closely I looked at the last one, but this looks HORRID! There is simply too much code here. The problem space is simple, make a simple solution. Start by just doing a uid allocator. This allocator can do its own 'sid->name' call internally if it helps implementation, as long as it can work without that. The concequeces of the DC being down should be considered however. (Possibly take a dump at regular intervals). This should be a simple if statement in the existing winbind uid allocation code. The issue of 'is this a winbind uid/gid' are easily solved by adding a request that asks specificly that to the winbind deamon. This should be configured by an smb.conf paramter, that will allow winbind to return UID's found in (for example) the SFU ADS schema, or a unix LDAP server etc. For now, don't worry about the cost of doing a socket call. Try and generate the smallest possible patch that fixes your problem. Andrew Bartlett -- Andrew Bartlett [EMAIL PROTECTED] Manager, Authentication Subsystems, Samba Team [EMAIL PROTECTED] Student Network Administrator, Hawker College [EMAIL PROTECTED] http://samba.org http://build.samba.org http://hawkerc.net
[PATCH] winbind id assignment module
I have created a nearly new implementation of the winbindd uid/gid assignment plugin that I first released about a week ago[1]. You can get the latest patch and sample module at http://www.cae.wisc.edu/~gerdts/samba/. Key implementation changes in this one are the following: * Based off of SAMBA_2_2 rather than release-2-2-4. * Now the shared library portion of the implementation is responsible for figuring out how a sid translates to a UID. Previously winbindd would try to resolve the domain and user/group names, then pass those to the module. This was to address problems alluded to at [2]. * Allows the module writer to control the order that smbd uses for resolving user and group names, as mentioned at [3]. * Ugliness was introduced that causes winbindd to require libsmbclient.so. It has only mildly been tested on Solaris 8, with SAMBA_2_2 checked out on or about 5/14/2002. References -- 1. http://lists.samba.org/pipermail/samba-technical/2002-May/036658.html 2. http://lists.samba.org/pipermail/samba-technical/2002-May/036731.html 3. http://lists.samba.org/pipermail/samba-technical/2002-May/036728.html Please let me know of success and/or failure that you experience with this patch. Mike
