You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Garrett Rooney <ro...@electricjellyfish.net> on 2005/08/15 02:52:03 UTC
[PATCH] use arrays in smtpd_request_rec (was Re: smtpd_request_rec
questions)
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.
Re: [PATCH] use arrays in smtpd_request_rec (was Re: smtpd_request_rec questions)
Posted by Rian Hunter <ri...@MIT.EDU>.
On Aug 14, 2005, at 11:08 PM, Garrett Rooney wrote:
> Rian Hunter wrote:
>
>> 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.
>>
>
> Well, it depends on what your general attitude towards checking for
> errors in memory allocation. In many projects it's generally
> considered to be the kind of error you can't effectively recover
> from anyway, so cluttering up the code with if (foo == NULL) checks
> is kind of pointless, you're likely to have been killed by a kernel
> OOM checker before that can do anything useful, or you could be on
> an OS that doesn't even return NULL (memory overcommit), so the
> checks are pointless anyway. The only way to be safe is to make
> sure that algorithmicly your program can't allocate unbounded
> amounts of memory, then tune your box and app so that this kind of
> problem doesn't happen in practice.
>
> APR generally doesn't bother checking for this kind of error for
> just this reason, same with Subversion and if I'm not mistaken
> Apache HTTPD itself.
>
> -garrett
>
Thanks for this information! After looking at code in httpd it seems
this is the case. I'll change the mod_smtpd code to reflect this
convention.
-rian
Re: [PATCH] use arrays in smtpd_request_rec (was Re: smtpd_request_rec
questions)
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
Rian Hunter wrote:
> 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.
Well, it depends on what your general attitude towards checking for
errors in memory allocation. In many projects it's generally considered
to be the kind of error you can't effectively recover from anyway, so
cluttering up the code with if (foo == NULL) checks is kind of
pointless, you're likely to have been killed by a kernel OOM checker
before that can do anything useful, or you could be on an OS that
doesn't even return NULL (memory overcommit), so the checks are
pointless anyway. The only way to be safe is to make sure that
algorithmicly your program can't allocate unbounded amounts of memory,
then tune your box and app so that this kind of problem doesn't happen
in practice.
APR generally doesn't bother checking for this kind of error for just
this reason, same with Subversion and if I'm not mistaken Apache HTTPD
itself.
-garrett
Re: [PATCH] use arrays in smtpd_request_rec (was Re: smtpd_request_rec questions)
Posted by Rian Hunter <ri...@MIT.EDU>.
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;
> }
>
Re: [PATCH] use arrays in smtpd_request_rec
Posted by Rian Hunter <ri...@MIT.EDU>.
On Aug 15, 2005, at 10:05 PM, Garrett Rooney wrote:
> Joe Schaefer wrote:
>
>> Garrett Rooney <ro...@electricjellyfish.net> writes:
>>
>>> Index: smtp_protocol.c
>>> ===================================================================
>>> --- smtp_protocol.c (revision 232680)
>>> +++ smtp_protocol.c (working copy)
>>>
>> [...]
>>
>>> + for (i = 0; i < sr->extensions->nelts; ++i) {
>>> + ap_rprintf(r, "%d-%s\r\n", 250, ((char **)sr->extensions-
>>> >nelts)[i]);
>>>
>>
>> ^^^^^
>> That should be "elts", shouldn't it?
>>
>
> Yes indeed, it should. One of the problems with data structures
> that require casting in order to access what you've stored in
> them... The version that Rian committed is slightly different, but
> still has the same problem. I'd post a patch, but it's probably
> faster to fix it by hand than it is to detach a patch from an email
> and apply it.
>
> Over in Subversion-land we have a couple of macros defined for
> dealing with apr arrays, they make it rather difficult to make this
> kind of mistake, and I'd love to see them in the main APR release,
> but the last time it was brought up I believe someone objected to
> their inclusion...
>
> -garrett
>
Fixed. I didn't even catch this because I don't test mod_smtpd with a
plugin that registers extensions. Things like this beckon some kind
of test suite module. Someone can get to this after mod_smtpd is redone.
-rian
Re: [PATCH] use arrays in smtpd_request_rec
Posted by Paul Querna <ch...@force-elite.com>.
Garrett Rooney wrote:
[...]
>>> + for (i = 0; i < sr->extensions->nelts; ++i) {
>>> + ap_rprintf(r, "%d-%s\r\n", 250, ((char
>>> **)sr->extensions->nelts)[i]);
>>
>>
>> ^^^^^
>>
>> That should be "elts", shouldn't it?
>>
>
> Yes indeed, it should. One of the problems with data structures that
> require casting in order to access what you've stored in them... The
> version that Rian committed is slightly different, but still has the
> same problem. I'd post a patch, but it's probably faster to fix it by
> hand than it is to detach a patch from an email and apply it.
>
> Over in Subversion-land we have a couple of macros defined for dealing
> with apr arrays, they make it rather difficult to make this kind of
> mistake, and I'd love to see them in the main APR release, but the last
> time it was brought up I believe someone objected to their inclusion...
I don't object, it sounds like a bloody good idea to me.
-Paul
Re: [PATCH] use arrays in smtpd_request_rec
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
Joe Schaefer wrote:
> Garrett Rooney <ro...@electricjellyfish.net> writes:
>
>
>>Index: smtp_protocol.c
>>===================================================================
>>--- smtp_protocol.c (revision 232680)
>>+++ smtp_protocol.c (working copy)
>
>
> [...]
>
>>+ for (i = 0; i < sr->extensions->nelts; ++i) {
>>+ ap_rprintf(r, "%d-%s\r\n", 250, ((char **)sr->extensions->nelts)[i]);
>
> ^^^^^
>
> That should be "elts", shouldn't it?
>
Yes indeed, it should. One of the problems with data structures that
require casting in order to access what you've stored in them... The
version that Rian committed is slightly different, but still has the
same problem. I'd post a patch, but it's probably faster to fix it by
hand than it is to detach a patch from an email and apply it.
Over in Subversion-land we have a couple of macros defined for dealing
with apr arrays, they make it rather difficult to make this kind of
mistake, and I'd love to see them in the main APR release, but the last
time it was brought up I believe someone objected to their inclusion...
-garrett
Re: [PATCH] use arrays in smtpd_request_rec
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Garrett Rooney <ro...@electricjellyfish.net> writes:
> Index: smtp_protocol.c
> ===================================================================
> --- smtp_protocol.c (revision 232680)
> +++ smtp_protocol.c (working copy)
[...]
> + for (i = 0; i < sr->extensions->nelts; ++i) {
> + ap_rprintf(r, "%d-%s\r\n", 250, ((char **)sr->extensions->nelts)[i]);
^^^^^
That should be "elts", shouldn't it?
--
Joe Schaefer