Comments on remaining FastMail.FM patches

[Bron Gondwana]

On Tue, 09 Jan 2007 16:37:23 -0500, "Ken Murchison" <murch at andrew.cmu.edu> said:
> I've been looking at the remaining, non-site specific patches, and here 
> are some comments.
> 
> Command timer - Assuming that we want to specify the minimum time as 
> fractional seconds (which I gather is the case given that its a double), 
> I'd prefer to have it specified as millisecond and use an integer rather 
> than a string in imapd.conf.  Since Jeff tells me that he'd like to have 
> this patch, I'm going to go ahead and make this change unless there is 
> an objection from the list.

I'm pretty sure we don't mind this.

> Fast Index Interator - Since the patch assumes that the sequence set is 
> sorted low->high, we don't get any advantage for SEARCH or UID EXPUNGE. 
>   Would it make more sense to parse the sequence set once, creating a 
> linked list of sorted ranges, and then do index_insequence() on the 
> linked list?  This would then would for STATUS, SEARCH, and UID EXPUNGE, 
> since all of the current code currently has msgno for each call 
> monotonically increasing.  If we're slick, we can remove nodes from the 
> head of the sequence set list, once we match a msgno that is greater 
> than the range in the node.

Yeah - I was thinking something similar.  The "cyrus way of doing things"
would probably be to generate a "rock" that contained a pointer to the
original string and some custom data structure that made it efficient.
Besides, it's almost object-oriented:

sequencelookup_t rock = sequencelookup_init(char *sequence);

for (i = 0; uid = uids[i], i++) {
  if (sequencelookup_check(rock, uid)) {
    r = do_stuff(uid);
    if (r) break;
  }
}

sequencelookup_free(rock);


This interface would allow you to reimplement those three functions and
one datatype with whatever implementation seemed most efficient.  Indeed,
you could write a really basic one with a couple of typedefs and #defines
that just implemented the current code unchanged.

> Accept 'From ' header from IMAP clients - I'm really reluctant to add 
> code to work around non-RFC2822 compliant messges, but if this is a big 
> deal for people, I could probably be convinced to make this an 
> imapd.conf option (probably another *_strict option).

I'm afraid these clients aren't going away any time soon, and their users
tend to get grumpy and go join some service that works how they expect if
you don't support them.  It's a harsh reality out here in the commercial
service world.

> Index Upgrade during Reconstruct - Is this a workaround for a bug in the 
> stock code?

Yeah, ish.  It's a workaround for Reconstruct opening an index and the
upgrade happening automatically, but the upgrade code trying to do things
like lock some pop3 constructs that aren't available in reconstruct.  The
other way to fix it would be to provide those constructs, but that seems
a bit pointless since you've already locked the index when you're
reconstructing.

It's only an issue across upgrades where the index format is changed.

> Longer constants for word sizes - We should probably make these values 
> (including MAXLITERALSIZE) configurable.

Yeah, makes sense.  This patch was written before I started here, and I
think the idea was quick and easy, not necessarily planned for upstream.

> Mailwasher bug workaround.  - The [CAPABILITY] response is just part of 
> the banner.  Mailwasher should just ignore whatever it doesn't 
> understand.  My guess would be that the size of the banner is 
> overflowing a static buffer.

Yeah, maybe.  Again - annoying your clients doesn't get you any brownie
points, and in this case it's a matter of "conservative in what you send
just in case your client isn't liberal in what they accept".  I said
pretty much exactly the same to our users and they said "so what, it used
to work, fix it" - basically.

> Statuscache - We've discussed this before, and I'm pretty sure its a 
> good idea.  I'd like to think some more to see if there might be a 
> better solution.

Fair enough.  This is Rob's baby.  We have noticed that it has timing
issues due to only 1 second resolution on some things.

Bron.
-- 
  Bron Gondwana
  brong at fastmail.fm