Oiy! This is a long one! I'm starting to wish that I had payed attention in typing class. :) It's all good content though. See comments below:
----- Original Message ----- From: "Kurt Bigler" <[EMAIL PROTECTED]> To: <[EMAIL PROTECTED]> Sent: Saturday, March 01, 2003 10:46 PM Subject: Re: [sqwebmail] Re: NEW: logindomainlist patch (2nd revision) > on 2/28/03 3:42 PM, Jesse Guardiani <[EMAIL PROTECTED]> wrote: > > > On Friday 28 February 2003 18:09, Kurt Bigler wrote: > >> on 2/28/03 12:44 PM, Jesse Guardiani <[EMAIL PROTECTED]> wrote: <snip> > I certainly don't mind if you do the work! I have no real need for glory > (referring to your point below), and don't expect much glory from putting > icing on the cake of what you did. Well, I would imagine that it depends on the work being done. If we're just talking about logic here, then I might as well do it since I screwed it up in the first place. If we're talking about a genuinely _NEW_ feature (and not just a fix to my screwy logic code), then I'll let you implement it. (Unless I really need it, but I'm actually pretty happy with the code as is. I can currently implement logindomainlist rewrite rules for ALL of my domains on two different machines with just three lines in each file. That ain't bad.) > > > But, I really wouldn't mind having you look the code over. You haven't stated > > your aptitude or experience level in C yet, but if you're capable it never > > hurts > > to proof read. > > I guess I have been programming in mostly C and C++ for 20 years. Alright, well, I already had a good bit of respect for you because of your critiques, but now I realize that it's well founded. Thanks for taking the time to critique me and my code. This logindomainlist functionality wouldn't be half as useful without your input. > The last > 10 years have been mostly C++ years, but recently stepping back into C for > unix server work was not at all alien. In the SqWebMail code I didn't see > any ways that C++ would have benefited it, and I was completely comfortable > working in the C. > > I would say that I usually find things wrong with almost any code I look at > - almost instantly - a bug at a glance every 30 lines or so. I don't find bugs so easily, but I do generally dislike other people's coding styles. I personally find myself wishing that the sqwebmail code was more heavily commented at times, but people who read my patch may find themselves wishing that I didn't comment so much! I'm still trying to find a happy medium, I guess. > This probably > reflects badly on the code I have had to work with, but I didn't find > anything wrong in the SqWebMail code I looked at, which is kind of a relief. > This includes your code from your previous patch, which I reviewed > completely. > > So, I'll take a look as soon as I can, and then we can go on from there. Sounds good. > > > > > If you wanna add something while you're in there, then go for it. Submit it > > to the list. If they like it, then it's golden. If they don't, they'll tell > > ya why. :) Kinda like you have with me this last week or so. > > I think I'll take the approach of doing the proposed coding (if any is > needed) myself, then passing it to you. You are aware of some things that > I'm not, like ISO/IEC 9899:1990 compliance. Since you have more vested > interest in this particularly piece of code it is probably good if you make > the final pass of clean-up edits before the next patch is posted. OK. BTW, as I stated before, I really don't have a clue what ISO/IEC 9899:1990 is good for in the real world. I just heard from a list programmer once that the first thing to look for is POSIX compliance, and then anything else is a good thing. I think POSIX only deals with actual program output and options. The ISO/IEC stuff is about the source, I believe. I know that the functions I used are ISO/IEC 9899:1990 compliant because I read the man pages when I was looking for a string tokenizing function. > > >> Here is what I propose. Let me test your latest, look at your code, and > >> see if I can come up with some minor changes to that code that will > >> accomplish what I am suggesting. Then I'll run those changes by you first. > >> If you find disadvantages, then we just forget it. If it turns out ok, > >> you can leave the next patch release to me if you want, or do it yourself. > > > > Your code, your release. Your glory. > > Well, as I said, I'm not really adding much in the way of functionality (if > anything). No glory expected. OK. Understandable. I still think I'll probably code the below fixes myself. After all, I messed them up in the first place. <snip> > >> (1) it allows the allvirtual option to go away > > > > Went away already. I'm currently just looking for an asterisk in the modifier > > field. Not sure if I mentioned this in my release notes, but I meant to > > mention > > it in the docs which I haven't yet gotten around to writing. > > No I don't think you mentioned it. I'm curious how this came about. Did it > require changes to your wildcard matching or did it just fall out of the > code you already had? It just 'fell' out. I didn't see any value in adding it while I was writing the code, so I just silently changed it. I don't think too many people are using this patch yet, so no harm done. I think (by the tone of our conversation) that this patch is starting to stabilize. I think the next release may be the last. We'll see. I was hoping to have it ready by the time Sam released his latest bug fixes, but I guess I'll just have to wait since he released one this morning. :-] > > >> (2) it avoids needing to add what you were describing 2 days ago as the '-' > >> modifier > > > > How so? I thought that modifier was pretty darn important. > > One of us is missing something here. I'm just going to jump in with my > assumptions and you can correct me as needed. We might have to kick this > back and forth a couple of times to get it clear. I think you may be right. We may have to bounce this back and forth a few times. But that's what it's all about, right? I'm looking forward to some nice solid code in the end. > > The way I _think_ it could/should work is that the first matching line in > logindomainlist determines everything. No other lines have any effect. > Sequence of lines only matters in that it determines which match will be > chosen when multiple items might match. The 2nd field determines the match > and if it contains wildcards and wildcards are enabled, it also determines > the value of any * occurring in the first field. Then the remaining fields > determine the result: > > the 1st field determines the default domain for login > and may be empty to specify that no default should be used > (this is not a new idea is it?) Not a new idea. But also not properly implemented in the current patch. I'll have to fix this and release a new patch. > > the 3rd field determines how the default domain will be used, > @ = just sets the login domain, and shows it, no popup list > otherwise = sets the default for the popup list Sure, you could use the '=' character, but currently it's just: 'anything besides the '*', '@', and '-' characters creates a drop down and defaults it to the defaultdomain. > > As I see it, the '-' option makes any value in the first field irrelevant. Yup. And the second field too. > Correct me if I'm wrong but I think it is not used for anything - you did I use the '-' modifier currently, but only because the blank first field doesn't work yet. :) > say no hidden fields, no drop-down, and as I see it there is no other use > for a login domain, so it does not even make sense to HAVE a value in the > first field - there is nothing to put there that means anything at all. Yeah the current code just ignores everything if there is a '-' in the modifier field. > So > it might as well be empty, and the fact that it is empty might as well be > the flag that there is no default domain. Yup. You're right. In hindsight I think this is much more logical too. Like you said before, I'm a one-change-at-a-time kind of guy. The '-' option put this functionality into the code and made it useable. Now I can clean it up and make useable become logical. > > The only reason I could think for introducing a _related_ flag might be to > provide a way to show the popup list but default it to the blank item at the > top. A blank first field should actually (in my mind) do this too. Just add a record with the necessary group identifier in the modifier field and insert a blank first field. Instant no default. Of coarse, this doesn't work yet either... Does that sound right? > > Your original comment was "If you're using wildcarding, then it wouldn't be > supported." referring to the empty first field which normally (or so I > thought) means that the popup and the hidden field should both be > suppressed. And I thought there was no need for the wildcarding case to be > any different from the non-wildcard case: empty 1st field means turn off > the features. As I recall, this "original" functionality can result in 4 > ways: > (1) no logindomainlist file > (2) no match found in the logindomainlist file Yeah. That's right. > (3) matching entry has first field empty Not implemented yet. > (4) matching entry enables popup list, and the blank entry is chosen Not implemented yet. > > (Possible notes there for the documentation.) I agree. > > This is also related to my thought that the first field should not have to > contain a * even if the 2nd field (then used only for matching) does. The first field doesn't have to contain an asterisk currently. I thought it would be a good idea to make the code as 'dummy proof' as possible, and I could imagine the users yelling at me that their logindomainlist file didn't work because they forgot the asterisk. In addition, the second field doesn't have to contain an asterisk either! This was implemented for the same reason as the first field. This raises an interesting question: Do we need the '@' modifier? I know you've asked that before and I declined to remove it. However, I'm considering it now. The '@' modifier adds a little bit of speed to the parsing process for strict '@' domains. However, I don't see speed being as much of an issue now with the wildcard rewrite rules in place. As I mentioned above, I can successfully rewrite ALL of my domains with just three logindomainlist lines per machine. I just can't see this file growing to over 100 lines if the user comprehends the wildcard rules. > I > know this isn't regular expressions, but the concept is analogous anyway: > regular expression substitution does not REQUIRE that a wildcard field int > he match string be referred to in the substitution string. > > This leads into the next item... > > >> (3) it probably allows one more feature, important or not, that I don't > >> think was brought up, which is to map several web domains via wildcard onto > >> a single mail domain - perhaps useful if you have a .com/.org/.net combo > >> but don't have aliased mail domains > > > > You can map .com/.org.net combos already. Say the domain is bob.com. If you > > put: > > > > bob.*:bob.*:* > > No I was really talking about: > > bob.com:bob.*:* This works currently. Try it yourself. This is functionality that I may use in the future, and contemplating it is one of the things that led me to implement non wildcard functionality for both the first and second fields under the '*' modifier. I didn't mention it in the release email because I though that only the people who followed our conversations closely would assume it wouldn't work. Everyone else will probably think it's only natural. <snip> > I've installed your latest now, but haven't looked at the code yet. Do you > think it is supposed to meet the wildcarding criterion as I described it > originally? The main point would be not to require the * in the first > field, and to let the * appear in any context in the 2nd field. It looks > now like the 2nd requirement is probably met, which I am guessing is what > let you get rid of the allvirtual option. I think the allvirtual thing was just common sense once I started coding. "Keep It Simple, Stupid", you know? <snip> > I'll look at the code soon, maybe tonight. I'll get back to you directly, > off-list, with any feedback on the code, since I figure this is not really a > coding forum. I don't expect any "coding" feedback per-se. More likely, I > might have feedback on the logic. I agree. I think this is more of a user support list. However, since there is no '-workers' list to speak of, I don't think it's that big of a deal. And the users have definately helped me out with the logic side of things. > > But it sounds almost as if most of my concerns might have vanished and I > have nothing to do. I still want to get clear about the '-' option. Yes. One final comment here. The '-' option is probably going to be technically complicated to remove. The reason for this is that the strtok() function isn't as well implemented as it should be (on FreeBSD anyway). If we have a string like this in logindomainlist: :domain.com:* The strtok() function will return 'domain.com' as field one. Where logically field one should be empty. I'll either have to wrap the strtok() function, or implement a sqwebmail_strtok() function with modified strtok() code copied from my system's source code. I'll see which is easiest, but I'm not all that opposed to just including an sqwebmail specific version of strtok(). I've always been a little worried about it's portability. I don't know how many systems conform to the ISO/IEC 9899:1990 standard, or even what is covered in that standard. Thanks for reading, Kurt! I appreciate the input. Together we're making this patch a real solid piece of work. I'm sure sqwebmail users will appreciate our the functionality for years to come. Jesse > > Thanks, > > Kurt Bigler > > >
