On 02/09/2013 01:50 AM, Todd Lyons wrote:
> I have finished coding up my first draft of DMARC support into Exim
[...]
>
> I welcome anybody to look over the code, to test it, etc.
acl.c
dmarc_has_been_checked variable
type should be bool.
is the operation safe for multiple mails per smtp conn? (also ~enable_forensic et.al.).
case ACLC_DMARC_STATUS (also ACLC_DKIM_STATUS
(style nit) space after comma for function args.
dmarc.c
general
style - function return type on separate line, zero first-level indent
of code within function, to match the rest of exim coding.
dmarc_init()
style - function opening { on new line.
Why ifdef EXPERIMENTAL_SPF ? Doesn't the whole facility depend on it?
Should the error cases shortcircuit further processing?
Could the function type be void? Also other functions in file.
"Skip DMARC if connection is SMTP Auth" - Discuss. Should this be
hardwired in the code, is it specified by a reference standard? Could
the test, if retained, be earlier in the function?
switch(libdbm_status) - consider a tabled-based coding.
"DMARC results:" log line - doesn't look easy to correlate with other log lines
for the message, on a busy server. Does it get the exim-id also?
Library shutdown calls - how expensive are these? Could they be opened
by the exim daemon process and just used by the child process(es) for the
message (are the libs safe for that)?
dmarc_write_history_file()
Why is history_buffer global?
Why is history_file_fd static?
Multiple string_sprintf() calls looks inefficient.
receive.c
#ifndef DISABLE_DKIM expansion to line 3429 - defining DISABLE_DKIM
now disables important code... Looks buggy. Has the testsuite been run?
--
Cheers,
Jeremy
