You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Cliff Woolley <JW...@wlu.edu> on 2000/07/22 04:49:39 UTC
[PATCH] buckets: make API a little cleaner
I was going to submit a patch to remove ap_get_bucket_char_str() and
ap_get_bucket_len(), since the same effect can be had by simply using
b->getstr(b) and b->getlen(b), as is done with the split and insert
functions, since that seems to be the current trend. But then I
realized that b->getlen(b) is just really, really ugly... the end-coders
shouldn't have to have any knowledge of the internals of the bucket
structure. Besides, ap_get_bucket_char_str(b) just "feels" and "looks"
cleaner, anyway.
So what I'm submitting instead is a patch to (re)add wrapper functions
for b->split() and b->insert() to make them match, and changing the
names of ap_get_bucket_char_str() and ap_get_bucket_len() to be a little
less cumbersome:
ap_bucket_getlen(b)
ap_bucket_getstr(b)
ap_bucket_split(b, nbyte)
ap_bucket_insert(b, buf, nbyte, w)
instead of
ap_get_bucket_len(b)
ap_get_bucket_char_str(b)
b->split(b, nbyte)
b->insert(b, buf, nbyte, w)
How's that?
Also, while I'm thinking about it, I notice that in the patched
http_core.c, there are lots of calls to b->insert() right after bucket b
is created. Wouldn't it be easier to have parameters to the bucket
creation function that would create a bucket and populate it at the same
time? This seems to make particular sense in the case of bucket colors
like mmap and rmem, where the data can only be "insert"ed once anyway.
It's possible that in these cases, b->insert could actually be NULL,
since we really don't want people inserting stuff into these buckets
manually. They're read-only; they can only have data stuffed into them
at the moment they're created... but I won't make that particular change
just yet.
So ap_bucket_*_create() would become ap_bucket_*_create(buf, nbyte, w),
where buf could be NULL if you really do want an empty bucket. That
saves a function call each time you make a new bucket and have something
to put in it right away. I'm working on that patch. I'd post it now,
but in the process of fixing up the calls to b->insert() all over the
place, I realized that there were some problems with the *_split()
functions as I mentioned in my previous email. So I kind of need to fix
that all in one fell swoop, as they're so interrelated. Look for that
patch tomorrow.
Note that the patch to http_core.c below assumes that ryan.patch has
already been applied, of course. This should make it easy to regenerate
a working ryan.patch.
--Cliff
Index: lib/apr/buckets/ap_buf.c
===================================================================
RCS file: /home/cvspublic/apache-2.0/src/lib/apr/buckets/ap_buf.c,v
retrieving revision 1.16
diff -u -r1.16 ap_buf.c
--- lib/apr/buckets/ap_buf.c 2000/07/21 22:24:28 1.16
+++ lib/apr/buckets/ap_buf.c 2000/07/22 00:18:45
@@ -137,8 +137,8 @@
orig = vec;
e = b->head;
while (e && nvec) {
- vec->iov_base = (void *)ap_get_bucket_char_str(e);
- vec->iov_len = ap_get_bucket_len(e);
+ vec->iov_base = (void *)ap_bucket_getstr(e);
+ vec->iov_len = ap_bucket_getlen(e);
e = e->next;
--nvec;
++vec;
@@ -203,21 +203,47 @@
return APR_SUCCESS;
}
-APR_EXPORT(const char *) ap_get_bucket_char_str(ap_bucket *b)
+APR_EXPORT(const char *) ap_bucket_getstr(ap_bucket *b)
{
- if (b) {
+ if (b && b->getstr) {
return b->getstr(b);
}
return NULL;
-}
+}
-APR_EXPORT(int) ap_get_bucket_len(ap_bucket *b)
+APR_EXPORT(int) ap_bucket_getlen(ap_bucket *b)
{
- if (b) {
+ if (b && b->getlen) {
return b->getlen(b);
}
return 0;
-}
+}
+
+APR_EXPORT(ap_status_t) ap_bucket_split(ap_bucket *b, ap_size_t nbyte)
+{
+ if (b && b->split) {
+ return b->split(b, nbyte);
+ }
+
+ /* attempted to split a bucket of a
+ * color that cannot be split
+ */
+ return APR_BADARG; /* what is the correct return value here? */
+}
+
+APR_EXPORT(ap_status_t) ap_bucket_insert(ap_bucket *b, const void *buf,
+ ap_size_t nbyte, ap_ssize_t
*w)
+{
+ if (b && b->insert) {
+ return b->insert(b, buf, nbyte, w);
+ }
+
+ /* attempted to insert into a bucket of a
+ * color that does not support insertion
+ */
+ *w = -1; /* no data was written */
+ return APR_BADARG; /* what is the correct return value here? */
+}
APR_EXPORT(int) ap_brigade_vputstrs(ap_bucket_brigade *b, va_list va)
{
@@ -227,8 +253,7 @@
ap_ssize_t i;
if (b->tail && b->tail->color == AP_BUCKET_rwmem) {
- ap_bucket *rw;
- rw = b->tail;
+ r = b->tail;
/* I have no idea if this is a good idea or not. Probably not.
* Basically, if the last bucket in the list is a rwmem bucket,
* then we just add to it instead of allocating a new read only
@@ -241,14 +266,14 @@
break;
j = strlen(x);
- rv = rw->insert(rw, x, j, &i);
+ rv = ap_bucket_insert(r, x, j, &i);
if (i != j) {
/* Do we need better error reporting? */
return -1;
}
k += i;
- ap_bucket_brigade_append_buckets(b, rw);
+ ap_bucket_brigade_append_buckets(b, r);
}
}
@@ -259,7 +284,7 @@
j = strlen(x);
r = ap_bucket_rwmem_create();
- rv = r->insert(r, x, j, &i);
+ rv = ap_bucket_insert(r, x, j, &i);
if (i != j) {
/* Do we need better error reporting? */
return -1;
@@ -295,7 +320,7 @@
res = ap_vsnprintf(buf, 4096, fmt, va);
r = ap_bucket_rwmem_create();
- res = r->insert(r, buf, strlen(buf), &i);
+ res = ap_bucket_insert(r, buf, strlen(buf), &i);
ap_bucket_brigade_append_buckets(b, r);
return res;
Index: lib/apr/buckets/apr_buf.h
===================================================================
RCS file: /home/cvspublic/apache-2.0/src/lib/apr/buckets/apr_buf.h,v
retrieving revision 1.16
diff -u -r1.16 apr_buf.h
--- lib/apr/buckets/apr_buf.h 2000/07/20 16:14:24 1.16
+++ lib/apr/buckets/apr_buf.h 2000/07/22 00:18:45
@@ -191,11 +191,17 @@
APR_EXPORT(ap_status_t) ap_bucket_list_destroy(ap_bucket *e);
/* Convert a bucket to a char * */
-APR_EXPORT(const char *) ap_get_bucket_char_str(ap_bucket *b);
+APR_EXPORT(const char *) ap_bucket_getstr(ap_bucket *b);
/* get the length of the data in the bucket */
-APR_EXPORT(int) ap_get_bucket_len(ap_bucket *b);
+APR_EXPORT(int) ap_bucket_getlen(ap_bucket *b);
+/* split a bucket into two buckets */
+APR_EXPORT(ap_status_t) ap_bucket_split(ap_bucket *b, ap_size_t nbyte);
+
+/* insert data into a bucket */
+APR_EXPORT(ap_status_t) ap_bucket_insert(ap_bucket *b, const void *buf,
+ ap_size_t nbyte, ap_ssize_t
*w);
/* Create a read/write memory bucket */
APR_EXPORT(ap_bucket *) ap_bucket_rwmem_create(void);
--- main/http_core.c.ryan Fri Jul 21 20:05:02 2000
+++ main/http_core.c Fri Jul 21 20:04:11 2000
@@ -2890,7 +2890,7 @@
int len = 0;
while (dptr) {
- len += ap_get_bucket_len(dptr);
+ len += ap_bucket_getlen(dptr);
dptr = dptr->next;
}
@@ -2936,7 +2936,7 @@
*/
dptr = b->head;
while (dptr) {
- len += ap_get_bucket_len(dptr);
+ len += ap_bucket_getlen(dptr);
dptr = dptr->next;
}
if (len < MIN_SIZE_TO_WRITE && b->tail->color != AP_BUCKET_eos) {
Re: [PATCH] buckets: make API a little cleaner
Posted by rb...@covalent.net.
I am against this patch. It just adds a layer that is unnecessary, and it
is going to be taken out by the compiler anyway. I also dislike name
changes like this one.
Some more details are below.
> Also, while I'm thinking about it, I notice that in the patched
> http_core.c, there are lots of calls to b->insert() right after bucket b
> is created. Wouldn't it be easier to have parameters to the bucket
> creation function that would create a bucket and populate it at the same
> time? This seems to make particular sense in the case of bucket colors
Yes, it would make sense. But, it keeps us from creating a list of empty
buckets that can be re-used. This is an optimization that we can make
later. The other option is to have both create functions, but that just
makes the API a bit less clean.
> like mmap and rmem, where the data can only be "insert"ed once anyway.
> It's possible that in these cases, b->insert could actually be NULL,
> since we really don't want people inserting stuff into these buckets
> manually. They're read-only; they can only have data stuffed into them
> at the moment they're created... but I won't make that particular change
> just yet.
>
> So ap_bucket_*_create() would become ap_bucket_*_create(buf, nbyte, w),
> where buf could be NULL if you really do want an empty bucket. That
> saves a function call each time you make a new bucket and have something
> to put in it right away. I'm working on that patch. I'd post it now,
> but in the process of fixing up the calls to b->insert() all over the
> place, I realized that there were some problems with the *_split()
> functions as I mentioned in my previous email. So I kind of need to fix
> that all in one fell swoop, as they're so interrelated. Look for that
> patch tomorrow.
> -APR_EXPORT(const char *) ap_get_bucket_char_str(ap_bucket *b)
> +APR_EXPORT(const char *) ap_bucket_getstr(ap_bucket *b)
> {
> - if (b) {
> + if (b && b->getstr) {
> return b->getstr(b);
The getstr function can never be NULL, so this is an unnecessary check.
> -APR_EXPORT(int) ap_get_bucket_len(ap_bucket *b)
> +APR_EXPORT(int) ap_bucket_getlen(ap_bucket *b)
> {
> - if (b) {
> + if (b && b->getlen) {
Same here.
> return b->getlen(b);
> }
> return 0;
> -}
> +}
> +
> +APR_EXPORT(ap_status_t) ap_bucket_split(ap_bucket *b, ap_size_t nbyte)
> +{
> + if (b && b->split) {
> + return b->split(b, nbyte);
> + }
> +
> + /* attempted to split a bucket of a
> + * color that cannot be split
> + */
ALL buckets can be split. I dislike this function.
> + return APR_BADARG; /* what is the correct return value here? */
> + /* attempted to insert into a bucket of a
> + * color that does not support insertion
> + */
> + *w = -1; /* no data was written */
> + return APR_BADARG; /* what is the correct return value here? */
All buckets support insert. Again, this function isn't necessary.
The change I am considering for the function pointers, is to use a macro
to get at the function. The reason for this, is that I am considering
some new technology from FreeBSD. Basically, this would allow us to add
and replace function pointers in the bucket types, without hurting
backwards compatability. This technology is called kobj or newbus
(depending on where you look), and adds about 10ns to each function call.
I will keep the list posted as I get more time to look at this stuff.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------