httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rian Hunter <r...@MIT.EDU>
Subject Re: [PATCH] use arrays in smtpd_request_rec (was Re: smtpd_request_rec questions)
Date Mon, 15 Aug 2005 02:40:35 GMT
This patch looks good but I have some questions. You seem to use the  
returned pointers from apr_array_push without checking if they are  
NULL. Even in apr_array_push, apr_palloc is used without checking for  
NULL even though apr_palloc can definitely return NULL.

Because of that, I'm not sure whether or not you don't check for NULL  
on purpose. Could you explain? Thanks.
-rian


On Aug 14, 2005, at 8:52 PM, Garrett Rooney wrote:

> Garrett Rooney wrote:
>
>> Rian Hunter wrote:
>>
>>> Ah I didn't even realize the key allocation, I'll fix that. Thanks!
>>>
>>> The reason I don't use an apr_array_t or similar is that I  
>>> thought  that the number of elements in that type has to be fixed  
>>> and can't be  automatically extended and allocated on the fly, If  
>>> I'm wrong I can  change these structures to use apr_array_t (or  
>>> you can send in a  patch if you like).
>>>
>> APR arrays are variable length, you use apr_array_push and  
>> apr_array_pop to add and remove entries.  I'll throw together the  
>> patch sometime today.
>>
>
> And here's the patch.  This is totally untested, since I haven't  
> yet gotten around to making mod_smtpd do anything other than  
> compile yet, but it's pretty simple and should give you an idea of  
> what I meant.
>
> -garrett
>
> * mod_smtpd.h
>   (smtpd_request_rec::e_index,
>    smtpd_request_rec::r_index): removed.
>   (smtpd_request_rec::extensions,
>    smtpd_request_rec::rcpt_to): change from a hash to an array.
>
> * smtp_core.c
>   (smtpd_register_extension): push the extension onto the array.
>   (smtpd_clear_request_rec): allocate the new arrays.
>
> * smtp_protocol.c
>   (helo): remove ext and ext_next variables, iterate over the array
>    instead of doing backflips to iterate over the hash.
>   (rcpt): remove new_elt variable, just push the rcpt address onto
>    the array.
> Index: mod_smtpd.h
> ===================================================================
> --- mod_smtpd.h    (revision 232680)
> +++ mod_smtpd.h    (working copy)
> @@ -90,16 +90,13 @@
>      // by default smtp
>      smtpd_protocol_type extended;
>
> -    // current max index of the extension hash
> -    int e_index;
> -    apr_hash_t *extensions;
> +    apr_array_header_t *extensions;
>
>      // string of who this mail is from
>      char *mail_from;
> -    // current max index of the rcpt_to hash
> -    int r_index;
> -    apr_hash_t *rcpt_to;
>
> +    apr_array_header_t *rcpt_to;
> +
>      // hostname we were helo'd with
>      char *helo;
>    } smtpd_request_rec;
> Index: smtp_protocol.c
> ===================================================================
> --- smtp_protocol.c    (revision 232680)
> +++ smtp_protocol.c    (working copy)
> @@ -121,7 +121,6 @@
>
>  HANDLER_DECLARE(ehlo) {
>    int i = 0, retval = 0;
> -  char *ext = NULL, *ext_next;
>    smtpd_request_rec *sr = smtpd_get_request_rec(r);
>
>    if (buffer[0] == '\0') {
> @@ -161,21 +160,13 @@
>    }
>
>    // print out extension
> -  ext = apr_hash_get(sr->extensions, &i, sizeof(i));
>    retval = 250;
>
> -  if (ext) {
> +  if (sr->extensions->nelts) {
>      ap_rprintf(r, "%d-%s\r\n", 250, sr->helo);
> -
> -    while (1) {
> -      i++;
> -      if ((ext_next = apr_hash_get(sr->extensions, &i, sizeof(i)))) {
> -    ap_rprintf(r, "%d-%s\r\n", 250, ext);
> -      } else {
> -    ap_rprintf(r, "%d %s\r\n", 250, ext);
> -    break;
> -      }
> -      ext = ext_next;
> +
> +    for (i = 0; i < sr->extensions->nelts; ++i) {
> +      ap_rprintf(r, "%d-%s\r\n", 250, ((char **)sr->extensions- 
> >nelts)[i]);
>      }
>    } else {
>      ap_rprintf(r, "%d %s\r\n", 250, sr->helo);
> @@ -344,7 +335,6 @@
>    char *loc;
>    char *allocated_string;
>    int retval = 0;
> -  int *new_elt;
>
>    // need mail first
>    if ((sr->state_vector != SMTPD_STATE_GOT_MAIL) &&
> @@ -413,19 +403,8 @@
>    // add a recipient
>
>    if ((allocated_string = apr_pstrdup(sr->p, loc))) {
> -    new_elt = apr_palloc(sr->p, sizeof(int));
> +    (*((char **)apr_array_push(sr->rcpt_to))) = allocated_string;
>
> -    if (!new_elt) {
> -      ap_rprintf(r, "%d %s\r\n", 421, "Error: Internal");
> -      // out of memory close connection
> -      retval = 0;
> -      goto end;
> -    }
> -
> -    *new_elt = sr->r_index;
> -    apr_hash_set(sr->rcpt_to, new_elt,
> -         sizeof(*new_elt), allocated_string);
> -    sr->r_index++;
>      sr->state_vector = SMTPD_STATE_GOT_RCPT;
>
>      ap_rprintf(r, "%d %s\r\n", 250, "Ok");
> Index: smtp_core.c
> ===================================================================
> --- smtp_core.c    (revision 232680)
> +++ smtp_core.c    (working copy)
> @@ -127,11 +127,7 @@
>  SMTPD_DECLARE_NONSTD(void)
>  smtpd_register_extension(smtpd_request_rec *sr, const char *line)
>  {
> -  int *cur = apr_palloc(sr->p, sizeof(int));
> -  *cur = sr->e_index;
> -
> -  apr_hash_set(sr->extensions, cur, sizeof(*cur), line);
> -  (sr->e_index)++;
> +  (*((char **)apr_array_push(sr->extensions))) = apr_pstrdup(sr- 
> >p, line);
>  }
>
>  // how to reset the transaction
> @@ -154,10 +150,8 @@
>    sr->state_vector = SMTPD_STATE_GOT_NOTHING;
>    sr->tfp = NULL;
>    sr->extended = SMTPD_PROTOCOL_SMTP;
> -  sr->e_index = 0;
> -  sr->extensions = apr_hash_make(sr->p);
> -  sr->r_index = 0;
> -  sr->rcpt_to = apr_hash_make(sr->p);
> +  sr->extensions = apr_array_make(sr->p, 5, sizeof(char *));
> +  sr->rcpt_to = apr_array_make(sr->p, 5, sizeof(char *));
>    sr->mail_from = NULL;
>    sr->helo = NULL;
>  }
>


Mime
View raw message