Re: [PATCH] use arrays in smtpd_request_rec (was Re: smtpd_request_rec questions)
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
[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.
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 (was Re: smtpd_request_rec questions)
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,
Re: [PATCH] use arrays in smtpd_request_rec (was Re: smtpd_request_rec questions)
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
