Re: [exim-dev] Exim support for OpenDMARC

Góra strony
Delete this message
Reply to this message
Autor: Jeremy Harris
Data:  
Dla: exim-dev
Temat: Re: [exim-dev] Exim support for OpenDMARC
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