On Mon, 2005-08-22 at 23:37, Garrett Rooney wrote: > > 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.
Good point. I'll do this. Originally there was a reason why I didn't want it to be a APR array, but that reason doesn't apply anymore. > > } 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++). Well this is another thing from old code. Originally the state was a bit vector, but now it's just incremental values so the name should certainly change. > > +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. I'll make a seperate change for this. > > + 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. I'll make a completely independent commit for style problems. > > > - 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... I left that code in there just in case others knew how to change emulate the same behavior. It was more like a note, but I should have definitely commented on why it was there. > > -garrett Thanks a lot! In the future I'll be a lot more careful about code I commit. -rian