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
