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