Re: svn commit: r235759 - in /httpd/mod_smtpd/trunk: mod_smtpd.h smtp.h smtp_core.c smtp_protocol.c

[Garrett Rooney <ro...@electricjellyfish.net>]

soc-rian@apache.org 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

Re: svn commit: r235759 - in /httpd/mod_smtpd/trunk: mod_smtpd.h smtp.h smtp_core.c smtp_protocol.c

[Rian Hunter <ri...@mit.edu>]

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