Milter-greylist

Hi all,

I recently upgraded to milter-greylist 4.2.2. I was looking forward to 
using the feature of adding an id string to ACL entries and easily 
gathering detailed stats on which rules were matching.

However, it appears that the default syslog output includes the 
configuration line number only, and not the id. The 'stat' keyword could 
be used to create an additional logfile. But, I'd prefer to have that 
information in the existing maillog.

Attached is a patch to replace the logging of ACL line number with ACL 
id string (if an id is specified). It uses fstring_expand(..., "%a") to 
build the string.

I'd be happy to hear other suggestions of how to collect ACL statistics.

Regards,
John

Example syslog output where an id of 'CBL' is defined:
Apr 15 15:18:20 server milter-greylist: n3FKIIpx012305: addr 
[127.0.0.1][127.0.0.1] from <nobody@...> to <nobody@...> 
delayed for 01:00:00 (ACL CBL)

Example syslog output where no id is defined:
Apr 15 15:18:20 server milter-greylist: n3FKIIpy012305: skipping 
greylist because this is the default action, (from=<nobody@...>, 
rcpt=<nobody@...>, addr=example.com[127.0.0.1]) ACL 212(none)

John Thiltges <jthiltges2@...> wrote:

> Attached is a patch to replace the logging of ACL line number with ACL
> id string (if an id is specified). It uses fstring_expand(..., "%a") to
> build the string.

I committed it on head, you will have it in 4.3.1

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

John Thiltges <jthiltges2@...> wrote:

> Attached is a patch to replace the logging of ACL line number with ACL
> id string (if an id is specified). It uses fstring_expand(..., "%a") to
> build the string.

What tabout the performance impact of this change? Now we go to
fstring_expand() regardless of the existance of an ACL id string, and
that function at least performs two mallocs. Perhaps we should test for
it before entering fstring_expand()?

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On 04/16/2009 11:34 PM, manu@... wrote:
> What tabout the performance impact of this change? Now we go to
> fstring_expand() regardless of the existance of an ACL id string, and
> that function at least performs two mallocs. Perhaps we should test for
> it before entering fstring_expand()

Would it be better to make aclstr[] a larger static buffer and not use 
fstring_expand()?

Example:

char aclstr[HDRLEN];

if (priv->priv_sr.sr_acl_id)
    snprintf(aclstr, sizeof(aclstr), " (ACL %s)",
        priv->priv_sr.sr_acl_id);
else if (priv->priv_sr.sr_acl_line != 0)
    snprintf(aclstr, sizeof(aclstr), " (ACL %d)",
        priv->priv_sr.sr_acl_line);
else
    aclstr[0] = '\0';

-John

John Thiltges <jthiltges2@...> wrote:

> Would it be better to make aclstr[] a larger static buffer and not use
> fstring_expand()?
> 
> Example:
> 
> char aclstr[HDRLEN];
> 
> if (priv->priv_sr.sr_acl_id)
>     snprintf(aclstr, sizeof(aclstr), " (ACL %s)",
>         priv->priv_sr.sr_acl_id);
> else if (priv->priv_sr.sr_acl_line != 0)
>     snprintf(aclstr, sizeof(aclstr), " (ACL %d)",
>         priv->priv_sr.sr_acl_line);
> else
>     aclstr[0] = '\0';

Well, this gets suddently much less readable :-)

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On 04/17/2009 12:06 PM, manu@... wrote:
> John Thiltges <jthiltges2@...> wrote:
>   
>> Would it be better to make aclstr[] a larger static buffer and not use
>> fstring_expand()
>
> Well, this gets suddently much less readable :-)
>   
I agree on the readability.

How about changing fstring_expand() to work like snprintf()? Passing a 
destination buffer could (hopefully) avoid a malloc/free.

-John