From: Jakob H. <jh...@pl...> - 2007-08-23 00:34:31
Attachments:
odmr.starttls.patch
|
Hi, here's a patch to add STARTTLS to the ODMR protocol. (I was a little surprised yesterday when I noticed that it isn't already there...) Most of the code is copied from imap.c. I inserted it into the generic smtp code, so it could probably be used to deliver fetched mail via TLS (if somebody ever wanted to do that). I did run some tests, but not really thorough. Comments welcome. You may notice that the patch removes adding our hostname to the ctl->domainlist for ODMR. I think this was plain wrong and bothered me everytime I set up fetchmail with odmr. ODMR's default is a simple "ATRN" without any arguments, which means "fetch mail for all domains associated with the account", so we should stick with that. Additional notes: - The logging looks broken, with -v you get mixed ODMR and SMTP line prefixes, but that's not related to my patch. Fixing this would could probably easily be done by changing smtp_mode and using it as an index to an array { "SMTP", "LMTP", "ODMR", ... }. - What's with the formatting in odmr.c?? Mixed spaces and tabs, that's quite horrible... if you don't mind, I'll clean that up (to whatever the preferred format is). Regards, Jakob |
From: Matthias A. <mat...@gm...> - 2007-08-23 09:44:10
|
Jakob Hirsch schrieb am 2007-08-23: > here's a patch to add STARTTLS to the ODMR protocol. (I was a little > surprised yesterday when I noticed that it isn't already there...) Thanks. > Most of the code is copied from imap.c. I inserted it into the generic > smtp code, so it could probably be used to deliver fetched mail via TLS > (if somebody ever wanted to do that). Without looking at the code, is it possible to factor out the common code into tls.c? Copy & paste is always prone to leaving a bug in one place while the other has been fixed. I should like the crucial code to be in ONE place when reasonably possible even if that requires using callback hooks or function address arguments, so that we don't have to patch two or three places when a bug is found in once, and "copying" breaks that, and SSL/TLS code makes delicate decisions -- let's avoid any avoidable embarrassment in that area. > You may notice that the patch removes adding our hostname to the > ctl->domainlist for ODMR. I think this was plain wrong and bothered me > everytime I set up fetchmail with odmr. ODMR's default is a simple > "ATRN" without any arguments, which means "fetch mail for all domains > associated with the account", so we should stick with that. If it's still possible to add it manually, I don't mind, although I wonder if that makes the patch suitable for inclusion into 6.3. > Additional notes: > - The logging looks broken, with -v you get mixed ODMR and SMTP line > prefixes, but that's not related to my patch. Fixing this would could > probably easily be done by changing smtp_mode and using it as an index > to an array { "SMTP", "LMTP", "ODMR", ... }. Sunil knows that part of the code better than I do, but we can fix that if we catch all occurrences and switch() statements related to smtp_mode. (I wonder if that's a reasonable design anyways, but I'm loathe to change it in 6.3.) > - What's with the formatting in odmr.c?? Mixed spaces and tabs, that's > quite horrible... if you don't mind, I'll clean that up (to whatever the > preferred format is). Well, my canonical style (I'm using vim) is an indentation width of 4 columns per level and using a tabwidth of 8, I.e. it's zero or more tabs followed by zero or four spaces. Any reasonable editor should be capable of writing that, and if not, use "all spaces" and run the code through unexpand --first-only before diffing. Thanks. I hope to get around to do some bugfixing later tonight (European time) so that I can release 6.3.9 soon and then move on to 6.4. Thanks for your efforts again. -- Matthias Andree |
From: Jakob H. <jh...@pl...> - 2007-08-23 11:54:16
|
Quoting Matthias Andree: >> Most of the code is copied from imap.c. I inserted it into the generic >> smtp code, so it could probably be used to deliver fetched mail via TLS >> (if somebody ever wanted to do that). > Without looking at the code, is it possible to factor out the common > code into tls.c? Copy & paste is always prone to leaving a bug in one In general, you are right, of course. But the code is merely 60 lines and contains no heavy logic. The only SSL-specific stuff is a single call to SSLOpen(), the rest is more or less the same, with only some protocol specifics, esp. the rereading of the server's capabilities. I'm not sure if creating a new function that has to fulfill all the needs is that good, but I didn't look that close at the code. pop3.c contains just the same code, btw. >> You may notice that the patch removes adding our hostname to the >> ctl->domainlist for ODMR. I think this was plain wrong and bothered me >> everytime I set up fetchmail with odmr. ODMR's default is a simple >> "ATRN" without any arguments, which means "fetch mail for all domains >> associated with the account", so we should stick with that. > If it's still possible to add it manually, I don't mind, although I > wonder if that makes the patch suitable for inclusion into 6.3. Sure, "fetchdomains" works as before. This is only the first hunk of the patch, so it could be easily removed. >> Additional notes: >> - The logging looks broken, with -v you get mixed ODMR and SMTP line >> prefixes, but that's not related to my patch. Fixing this would could >> probably easily be done by changing smtp_mode and using it as an index >> to an array { "SMTP", "LMTP", "ODMR", ... }. > Sunil knows that part of the code better than I do, but we can fix that > if we catch all occurrences and switch() statements related to > smtp_mode. (I wonder if that's a reasonable design anyways, but I'm > loathe to change it in 6.3.) AFAICS smtp_mode is used in logging, to get the char, or in comparisons with the macroes SMTP_MODE and LMTP_MODE. Should be not too hard to change. >> - What's with the formatting in odmr.c?? Mixed spaces and tabs, that's >> quite horrible... if you don't mind, I'll clean that up (to whatever the >> preferred format is). > Well, my canonical style (I'm using vim) is an indentation width of 4 > columns per level and using a tabwidth of 8, I.e. it's zero or more tabs Oh. Ok. Any reason for that? (Not that I want to start a discussion on tabs vs. spaces.) And how do you tell vim (which I happen to use, too) to use this style? > Thanks for your efforts again. Well, it's definitely not much compared to your efforts. So, many thanks back :) |
From: Matthias A. <mat...@gm...> - 2007-08-23 13:24:09
|
Jakob Hirsch schrieb: > In general, you are right, of course. But the code is merely 60 lines > and contains no heavy logic. The only SSL-specific stuff is a single > call to SSLOpen(), the rest is more or less the same, with only some > protocol specifics, esp. the rereading of the server's capabilities. I'm > not sure if creating a new function that has to fulfill all the needs is > that good, but I didn't look that close at the code. pop3.c contains > just the same code, btw. Apparently my attempt to subliminally delegate this part of the cleanup failed, but at least I tried. :-) >> If it's still possible to add it manually, I don't mind, although I >> wonder if that makes the patch suitable for inclusion into 6.3. > > Sure, "fetchdomains" works as before. This is only the first hunk of the > patch, so it could be easily removed. It's an incompatible change, so if we can find a configuration option to suppress adding that, I'm more comfortable with that because it avoids another "incompatible change" changelog entry :-) >>> - What's with the formatting in odmr.c?? Mixed spaces and tabs, that's >>> quite horrible... if you don't mind, I'll clean that up (to whatever the >>> preferred format is). >> Well, my canonical style (I'm using vim) is an indentation width of 4 >> columns per level and using a tabwidth of 8, I.e. it's zero or more tabs > > Oh. Ok. Any reason for that? (Not that I want to start a discussion on > tabs vs. spaces.) > And how do you tell vim (which I happen to use, too) to use this style? Historical practice with unknown origin. I don't even know if smartindent/cindent/indentexpr would be even easier to use (-8 I think the crucial vim magic is the first 'set' line below (tabstop=8 is the default in my vim 7): " -------------------------------------------------------- " ~/.vimrc excerpt " crucial part: set shiftwidth=4 softtabstop=4 tabstop=8 " And beyond that, I use - among others: set nocompatible set bs=2 fo=cqrt ls=2 shm=at tw=72 ww=<,>,h,l set showmatch ruler set grepprg=grep\ -nH\ $* set hidden set backup set autoindent set iskeyword+=: filetype plugin on filetype indent on if has("syntax") so ${VIMRUNTIME}/syntax/syntax.vim endif " end of ~/.vimrc excerpt " -------------------------------------------------------- |