Another non-standard search/sort field :)

[Vladislav Bogdanov]

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?

>
> I'm kind of tempted to suggest using the prot.c prot_write, prot_read functions
> rather than raw fwrite and friends, but it's a real nit - they look fine.  THOUGH,
> I would test for errno if you're going to use them.  Running in a real process,
> you could signals.  The prot stuff hides that for you.
>
> I guess the nice thing is, it will just fail back to the MTA which will try again later,
> so no real damage gets done.

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.

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

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

>
> Thanks so much for posting it :)  I'll get Ellie to have a look at it (yay minions)
> and integrate it.
>
> Cheers,
>
> Bron.
>

Best,
Vladislav