[EMAIL PROTECTED] wrote:
Author: soc-rian
Date: Mon Aug 22 10:22:27 2005
New Revision: 235759
URL: http://svn.apache.org/viewcvs?rev=235759&view=rev
Log:
mod_smtpd overhaul:
1. new structs: smtpd_conn_rec, smtpd_trans_rec
2. different i/o API: smtpd_getline, smtpd_respond_oneline,
smtpd_respond_multiline.
A quick review. Note that all of this stuff would be much easier to
review and understand if there were some examples of how to write
modules that plug into it and make it do something. As I don't have
access to that, all of my comments are kind of theoretical, but
hopefully they can still be helpful.
-garrett
Modified:
httpd/mod_smtpd/trunk/mod_smtpd.h
httpd/mod_smtpd/trunk/smtp.h
httpd/mod_smtpd/trunk/smtp_core.c
httpd/mod_smtpd/trunk/smtp_protocol.c
Modified: httpd/mod_smtpd/trunk/mod_smtpd.h
URL:
http://svn.apache.org/viewcvs/httpd/mod_smtpd/trunk/mod_smtpd.h?rev=235759&r1=235758&r2=235759&view=diff
==============================================================================
--- httpd/mod_smtpd/trunk/mod_smtpd.h (original)
+++ httpd/mod_smtpd/trunk/mod_smtpd.h Mon Aug 22 10:22:27 2005
@@ -63,7 +63,7 @@
SMTPD_STATE_GOT_HELO,
SMTPD_STATE_GOT_MAIL,
SMTPD_STATE_GOT_RCPT
- } smtpd_request_state;
+ } smtpd_trans_state;
typedef enum {
SMTPD_PROTOCOL_SMTP,
@@ -72,78 +72,90 @@
typedef struct smtpd_return_data {
apr_pool_t *p;
- char *msg;
+ /* list of messages
+ null terminated */
+ char **msgs;
Why is this a char **? It seems kind of error prone to require the
module programmer to allocate and manage that array correctly, this
feels like another case where an APR array would potentially simplify
things.
} smtpd_return_data;
- typedef struct smtpd_request_rec {
+ typedef struct smtpd_trans_rec {
apr_pool_t *p;
- conn_rec *c;
- server_rec *s;
-
- /* spooled data file pointer */
- apr_file_t *tfp;
/* where are we in the current transaction */
- smtpd_request_state state_vector;
+ smtpd_trans_state state_vector;
Out of curiosity, why is this called a "state_vector", there's only one
element, so it doesn't feel much like a vector to me (at least not in
the std::vector sense of the word from c++).
+apr_status_t
+smtpd_getline(smtpd_conn_rec *scr, char *data, apr_size_t dlen)
+{
+ apr_status_t rc;
+ apr_bucket *e;
+ const char *str;
+ char *pos, *last_char = data;
+ apr_size_t len, bytes_handled = 0;
+
+ while (1) {
+ rc = ap_get_brigade(scr->c->input_filters, scr->bb_in, AP_MODE_GETLINE,
+ APR_BLOCK_READ, 0);
+ if (rc != APR_SUCCESS) return rc;
Putting the body of the if statement on the same line as the if is kind
of sketchy IMO. In particular it means that when you're single stepping
through the code in a debugger it's impossible to easily tell if the
statement is true or not based on the fact that you stepped onto that line.
Also, APR_SUCCESS is defined to be zero, so you can write that in less
characters by saying:
if (rc)
return rc;
Although that's just a style thing.
+ while(!APR_BRIGADE_EMPTY(scr->bb_in)) {
+ e = APR_BRIGADE_FIRST(scr->bb_in);
+
+ rc = apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
+ if (rc != APR_SUCCESS) return rc;
+ apr_bucket_delete(e);
+
+ if (len == 0) continue;
+
+ /* Would this overrun our buffer? If so, we'll die. */
+ if (dlen < bytes_handled + len) {
+ if (data) {
+ /* ensure this string is NUL terminated */
+ if (bytes_handled > 0) {
+ data[bytes_handled-1] = '\0';
+ }
+ else {
+ data[0] = '\0';
+ }
+ }
+ return APR_ENOSPC;
+ }
The indentation in this section seems kind of screwed up. Speaking of
indentation, none of the mod_smtpd code follows the httpd style
guidelines, if that's going to eventually change it might be good to get
it out of the way sooner rather than later, as those kind of changes
tend to obscure the revision history of the code. See
http://httpd.apache.org/dev/styleguide.html for details about the style
guidelines.
- apr_socket_t *csd=((core_net_rec *)r->input_filters->ctx)->client_socket;
+ // r->request_config = ap_create_request_config(r->pool);
C++ style comment. This will break some C compilers. Plus, it's an
example of leaving old code commented out in a function, which is a bad
habit to get into. If the code has a reason to be there, at least leave
a comment as to why, if not, just kill it.
- r = smtpd_create_request(c);
- ap_update_child_status(r->connection->sbh, SERVER_BUSY_KEEPALIVE, r);
+ scr = smtpd_create_conn_rec(c);
+ // ap_update_child_status(scr->c->sbh, SERVER_BUSY_KEEPALIVE, r);
Another case of that...
-garrett
