httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rian Hunter <r...@MIT.EDU>
Subject Re: mod_smtpd module review
Date Wed, 31 Aug 2005 15:21:03 GMT

On Aug 30, 2005, at 11:19 AM, Brian J. France wrote:

> This past week I have finished up a few modules and ready for review.
>
> http://www.brianfrance.com/software/apache/mod_smtpd_load.tar.gz
>
> mod_smtpd_load:
>   This module allows rejecting connection (temporarily) based on  
> server load
>   It is not very cross platform (any os with getloadavg), but I am  
> sure we
>   can work on that.
>
> I have finished mod_smtpd_access_dbi, but after talking with Paul  
> on IRC
> I need to convert it to use mod_dbd instead of mod_dbi_pool.   
> Working on that
> now and will post another message when done.  I could build a tar  
> ball of this
> module if anybody is interested as the the flow will not change,  
> just how the
> db connection is handled.
>
> mod_smtpd_access_dbi:
>     This module is similar to sendmails access file.  It allows
>     checking of the ip/hostname/from/to items via a database to see if
>     things should be rejected.  It uses mod_dbi_pool and libdbi.
>     Thanks to Paul for mod_dbi_pool and code examples from  
> mod_vhost_dbi.

This is great!

> A few things I have found while writing these modules. How about  
> changing smtpd_return_data from this:
>
> typedef struct smtpd_return_data {
>     apr_pool_t *p;
>     /* list of messages */
>     apr_array_header_t *msgs;
> } smtpd_return_data;
>
> to something like this:
>
> typedef struct smtpd_return_data {
>     apr_pool_t *p;
>     int code;
>     /* list of messages */
>     apr_array_header_t *user_msgs;
>     apr_array_header_t *log_msgs;
> } smtpd_return_data;
>
> While doing the mod_smtpd_load module I found when I want to deny a  
> connection I can set what message the user will get, but I also  
> want to log a different message instead of the default "Connection  
> Denied" (current I log my own and the default gets logged).  Of  
> course this might be another thread of how and what do we plan on  
> logging.
>

IMO I think that logging twice is fine. mod_smtpd logs "Connection  
Denied" because a server admin may want to know what connections are  
getting denied for debugging and etc. and I feel its absolutely  
necessary, same with the MAIL command. If your module wants to log  
extra information, I think that's OK.

Maybe for clarity all mod_smtpd's logs can be prefixed with  
"mod_smtpd:" and other modules can follow suit.

> I would also like to set the error code, because looking over  
> rfc0821 I think it should return 452 or may be that needs to be a  
> default for smtpd_run_connect soft errors (552 for hard errors).   
> Should we allow the module to set the error code?

Error codes can be sent with SMTPD_DONE. Otherwise error codes won't  
change in the next fifty years for things like SMTPD_DENY and  
friends. I'll look into changing those specific codes. Most of the  
error codes I got from Postfix and Qpsmtpd but of course they aren't  
perfect.

> In mod_smtpd_access_dbi I found it strange that I get a string that  
> looks like this: " <name@domain.tld>" instead of  
> "name@domain.tld".  For the mail/rcpt hooks should we send a struct  
> that has the full line sent, the data from the full line and the  
> parsed email address?  I have some code that duplicates the string  
> and then remove spaces and < from the beginning and > from the end,  
> but that seems like it should be done before my function is  
> called.  Another problem I can see is when we get into things like  
> the size options:
>
> MAIL FROM:<ned@thor.innosoft.com> SIZE=500000
>
> Do we want every module to have to parse the email address  
> (removing < >) and the SIZE?

This is a very good point. Honestly I'm not really sure since I'm not  
experienced with writing a plugin based architecture, but I'm almost  
positive that most people will say that mod_smtpd should parse the  
email address and options along with it. When I first wrote mod_smtpd  
I figured I'd pass the un-parsed string after from: verbatim to let  
the modules decide what to do with it. Maybe to allow some crazy  
things. Either way, having mod_smtpd parse sounds like the right idea  
so how about this hook for mail:

smtp_run_mail(smtp_conn_rec *, smtp_return_data *, char  
*parsed_address, apr_array_header_t *options);

The options won't be stored as name->value pairs: more like an array  
of strings like "SIZE=50000". Does that sound good?

Rian Hunter

Mime
View raw message