Another non-standard search/sort field :)

[Bron Gondwana]

On Fri, Sep 11, 2015, at 23:19, Vladislav Bogdanov wrote:
> 11.09.2015 14:49, Bron Gondwana wrote:
> > On Fri, Sep 11, 2015, at 20:00, Vladislav Bogdanov wrote:
> >> 11.09.2015 12:01, Bron Gondwana wrote:
> >>> On Fri, Sep 11, 2015, at 16:48, Vladislav Bogdanov wrote:
> >>>> 11.09.2015 03:52, Bron Gondwana wrote:
> >>>>> sort: spamscore search: spamabove / spambelow
> >>>>>
> >>>>> These use the X-Spam-score header which is a floating point number
> >>>>> with a single decimal place usually, i.e. 5.0, 17.3.  spamabove is GE
> >>>>> and spambelow is LT.
> >>>>>
> >>>>> I'm going to push this back, because it doesn't clash with anything.
> >>>>> It's kinda nice to be able to sort by spamscore to quickly put the
> >>>>> focus on the most likely to be be wrongly classified messages, and
> >>>>> we're going to support that in our interface at some stage.
> >>>>>
> >>>>> Bron.
> >>>>>
> >>>>
> >>>> Ah, I have a nice patch for spamtest extension against 2.4.17.
> >>>>
> >>>> It connects to spamd itself from lmtpd, checks the message and sets
> >>>> additional headers. Sieve integration is done too.
> >>>>
> >>>> Need to send it here.
> >>>
> >>> That would be great.
> >>
> >> Attached. Just found that it inconsistently uses tabs/spaces. I hope
> >> that is not an issue at least for initial review.
> >
> > Initial impressions:
> >
> > I would rename 's' to 'fd'.  Everyone knows what 'fd' does, s could be anything,
> > and being an int, it's totally un-typesafe.
> 
> It was initially written against newly-released 2.3.8. That time socket 
> fd was usually named 's' or 'sock'. 's' is shorter ;) Did that change?

Fair enough :)  I haven't dealt with that area of the code quite so much,
more the index and database filehandles.

> Yep, that never resulted in lost messages for last N+1 years in quite 
> busy setups (not as fastmail, but anyways...).
> 
> So, yes, that "works for me" and such change would be really cosmetic.
> 
> I just ported it to 2.4.17 recently without even looking much at the 
> code (but yes, that is tested on newer setups right from the patch date).
> 
> I can send previous revision (against 2.3.13) as well.

No, that's fine.  We can work with this.

> > I've got a sneaking feeling that your entire spamtest_parse_hosts could be
> > turned into a tight little piece of code based on strarray_split() - but it looks fine.
> 
> No opinion. It was not available in 2.3.x. IMHO spamtest_parse_hosts is 
> very straight-forward.
> 
> And, that _may_ conflict with the line in TODO list (which I probably 
> will never do anyways because I have no idea how to make that fair 
> enough without initial lookups):
> * Make load-balancing work if hostname that resolve to multiple A 
> records is used in "spamtest_spamd_hosts".

One interesting possibility (more for spam than virus checking) is to send requests
for the same user to the same host always, which would require some form of
consistent hashing.  We do that with nginx at FastMail for web requests:

upstream internalbackend {
  server web1.nyi.internal:8080;
  server web2.nyi.internal:8080;
  server web3.nyi.internal:8080 down;
  server web4.nyi.internal:8080;
  server web5.nyi.internal:8080;
  server web6.nyi.internal:8080;
}

So it knows web3 is down and hashes elsewhere, but when it comes back up, the
same users will move back.

We do the same for postfix lmtp delivery with some magic code in a thing called lmtpforwardd,
which just listens on localhost and forwards to the correct server based on the list of up spam
scan machines.

> > All the code looks like it works (which is not a surprise, because it's been used).
> > My main concerns would be around signal safety in the file IO syscalls.
> 
> Feel free to convert them to prot ones, but I do not feel it is strictly 
> required.

Sure thing :)

Bron.

-- 
  Bron Gondwana
  brong at fastmail.fm