You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2004/11/04 23:07:58 UTC
[PATCH] add ap_brigade_insert_file()
This factors out the logic for correctly inserting a file into a
brigade, doing the special little dance for large files if necessary;
the code was already duplicated twice and it's needed in other places
too. (pervasive largefile support being my top 2.1 goal :)
Any comments, naming preferences, "put it in apr-util you twit" flames?
Index: include/util_filter.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/util_filter.h,v
retrieving revision 1.92
diff -u -r1.92 util_filter.h
--- include/util_filter.h 31 Oct 2004 18:00:43 -0000 1.92
+++ include/util_filter.h 4 Nov 2004 21:53:57 -0000
@@ -489,6 +489,22 @@
apr_bucket_brigade **b, apr_pool_t *p);
/**
+ * Utility function to insert a file of given length onto the end of
+ * the brigade. The file is split into multiple buckets if it is
+ * larger than the maximum size which can be represented by a single bucket.
+ * @param bb the brigade to insert into
+ * @param f the file to insert
+ * @param start the offset into the file
+ * @param len the length of the file to insert
+ * @param p pool from which file buckets are allocated
+ * @return the last bucket inserted
+ */
+AP_DECLARE(apr_bucket *) ap_brigade_insert_file(apr_bucket_brigade *bb,
+ apr_file_t *f, apr_off_t start,
+ apr_off_t len, apr_pool_t *p);
+
+
+/**
* Flush function for apr_brigade_* calls. This calls ap_pass_brigade
* to flush the brigade if the brigade buffer overflows.
* @param bb The brigade to flush
Index: modules/experimental/mod_disk_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_disk_cache.c,v
retrieving revision 1.68
diff -u -r1.68 mod_disk_cache.c
--- modules/experimental/mod_disk_cache.c 4 Nov 2004 21:51:12 -0000 1.68
+++ modules/experimental/mod_disk_cache.c 4 Nov 2004 21:53:59 -0000
@@ -502,14 +502,10 @@
static apr_status_t recall_body(cache_handle_t *h, apr_pool_t *p, apr_bucket_brigade *bb)
{
- apr_bucket *e;
disk_cache_object_t *dobj = (disk_cache_object_t*) h->cache_obj->vobj;
- e = apr_bucket_file_create(dobj->fd, 0, (apr_size_t) dobj->file_size, p,
- bb->bucket_alloc);
- APR_BRIGADE_INSERT_HEAD(bb, e);
- e = apr_bucket_eos_create(bb->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(bb, e);
+ ap_brigade_insert_file(bb, dobj->fd, 0, dobj->file_size, p);
+ APR_BRIGADE_INSERT_TAIL(bb, apr_bucket_eos_create(bb->bucket_alloc));
return APR_SUCCESS;
}
Index: modules/generators/mod_asis.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/generators/mod_asis.c,v
retrieving revision 1.51
diff -u -r1.51 mod_asis.c
--- modules/generators/mod_asis.c 9 Feb 2004 20:29:19 -0000 1.51
+++ modules/generators/mod_asis.c 4 Nov 2004 21:53:59 -0000
@@ -76,7 +76,6 @@
if (!r->header_only) {
apr_bucket_brigade *bb;
- apr_bucket *b;
apr_off_t pos = 0;
rv = apr_file_seek(f, APR_CUR, &pos);
@@ -89,30 +88,8 @@
}
bb = apr_brigade_create(r->pool, c->bucket_alloc);
-#if APR_HAS_LARGE_FILES
- if (r->finfo.size - pos > AP_MAX_SENDFILE) {
- /* APR_HAS_LARGE_FILES issue; must split into mutiple buckets,
- * no greater than MAX(apr_size_t), and more granular than that
- * in case the brigade code/filters attempt to read it directly.
- */
- apr_off_t fsize = r->finfo.size - pos;
- b = apr_bucket_file_create(f, pos, AP_MAX_SENDFILE,
- r->pool, c->bucket_alloc);
- while (fsize > AP_MAX_SENDFILE) {
- APR_BRIGADE_INSERT_TAIL(bb, b);
- apr_bucket_copy(b, &b);
- b->start += AP_MAX_SENDFILE;
- fsize -= AP_MAX_SENDFILE;
- }
- b->length = (apr_size_t)fsize; /* Resize just the last bucket */
- }
- else
-#endif
- b = apr_bucket_file_create(f, pos, (apr_size_t) (r->finfo.size - pos),
- r->pool, c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(bb, b);
- b = apr_bucket_eos_create(c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(bb, b);
+ ap_brigade_insert_file(bb, f, pos, (apr_size_t) (r->finfo.size - pos),
+ r->pool);
rv = ap_pass_brigade(r->output_filters, bb);
if (rv != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
Index: server/core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/core.c,v
retrieving revision 1.292
diff -u -r1.292 core.c
--- server/core.c 4 Nov 2004 16:04:55 -0000 1.292
+++ server/core.c 4 Nov 2004 21:54:00 -0000
@@ -3686,34 +3686,14 @@
bb = apr_brigade_create(r->pool, c->bucket_alloc);
- /* For platforms where the size of the file may be larger than
- * that which can be stored in a single bucket (where the
- * length field is an apr_size_t), split it into several
- * buckets: */
- if (sizeof(apr_off_t) > sizeof(apr_size_t)
- && r->finfo.size > AP_MAX_SENDFILE) {
- apr_off_t fsize = r->finfo.size;
- e = apr_bucket_file_create(fd, 0, AP_MAX_SENDFILE, r->pool,
- c->bucket_alloc);
- while (fsize > AP_MAX_SENDFILE) {
- apr_bucket *ce;
- apr_bucket_copy(e, &ce);
- APR_BRIGADE_INSERT_TAIL(bb, ce);
- e->start += AP_MAX_SENDFILE;
- fsize -= AP_MAX_SENDFILE;
- }
- e->length = (apr_size_t)fsize; /* Resize just the last bucket */
- }
- else
- e = apr_bucket_file_create(fd, 0, (apr_size_t)r->finfo.size,
- r->pool, c->bucket_alloc);
+ /* insert the file as one or more buckets: */
+ e = ap_brigade_insert_file(bb, fd, 0, r->finfo.size, r->pool);
#if APR_HAS_MMAP
if (d->enable_mmap == ENABLE_MMAP_OFF) {
(void)apr_bucket_file_enable_mmap(e, 0);
}
#endif
- APR_BRIGADE_INSERT_TAIL(bb, e);
e = apr_bucket_eos_create(c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, e);
Index: server/util_filter.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
retrieving revision 1.102
diff -u -r1.102 util_filter.c
--- server/util_filter.c 1 Nov 2004 23:04:05 -0000 1.102
+++ server/util_filter.c 4 Nov 2004 21:54:00 -0000
@@ -619,3 +619,32 @@
{
f->frec->proto_flags = flags ;
}
+
+AP_DECLARE(apr_bucket *) ap_brigade_insert_file(apr_bucket_brigade *bb,
+ apr_file_t *f, apr_off_t start,
+ apr_off_t length, apr_pool_t *p)
+{
+ apr_bucket *e;
+
+ if (sizeof(apr_off_t) == sizeof(apr_size_t) || length < AP_MAX_SENDFILE) {
+ e = apr_bucket_file_create(f, start, (apr_size_t)length, p,
+ bb->bucket_alloc);
+ }
+ else {
+ /* Several buckets are needed. */
+ e = apr_bucket_file_create(f, start, AP_MAX_SENDFILE, p,
+ bb->bucket_alloc);
+
+ while (length > AP_MAX_SENDFILE) {
+ apr_bucket *ce;
+ apr_bucket_copy(e, &ce);
+ APR_BRIGADE_INSERT_TAIL(bb, ce);
+ e->start += AP_MAX_SENDFILE;
+ length -= AP_MAX_SENDFILE;
+ }
+ e->length = (apr_size_t)length; /* Resize just the last bucket */
+ }
+
+ APR_BRIGADE_INSERT_TAIL(bb, e);
+ return e;
+}
Re: [PATCH] add ap_brigade_insert_file()
Posted by Cliff Woolley <jw...@virginia.edu>.
On Thu, 4 Nov 2004, Joe Orton wrote:
> This factors out the logic for correctly inserting a file into a
> brigade, doing the special little dance for large files if necessary;
> the code was already duplicated twice and it's needed in other places
> too. (pervasive largefile support being my top 2.1 goal :)
Amen, brother. Thank you! :)
> Any comments, naming preferences, "put it in apr-util you twit" flames?
I'd have absolutely no objection to that. Sounds like a fabulous idea
actually.
--Cliff
Re: [PATCH] add ap_brigade_insert_file()
Posted by Thom May <th...@planetarytramp.net>.
* Joe Orton (jorton@redhat.com) wrote :
> This factors out the logic for correctly inserting a file into a
> brigade, doing the special little dance for large files if necessary;
> the code was already duplicated twice and it's needed in other places
> too. (pervasive largefile support being my top 2.1 goal :)
>
> Any comments, naming preferences, "put it in apr-util you twit" flames?
>
I suspect I'd like to see it in apr-util, but I'd hesitate to call you a
twit :-)
(and change the name accordingly)
Otherwise, looks good.
Cheers,
-Thom
Re: [PATCH] add ap_brigade_insert_file()
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:50 AM 11/6/2004, you wrote:
>--On Thursday, November 4, 2004 10:34 PM -0800 Justin Erenkrantz <ju...@erenkrantz.com> wrote:
>
>>>Any comments, naming preferences, "put it in apr-util you twit" flames?
>>
>>"Put it in apr-util, you marvelous angel." Pretty please with sugar on
>>top...
>
>Point to note that if it goes into APR, it'd have to be in APR-util 1.1. I'd be fine with making httpd depend on 1.1+, but...
But what?
It seems as though Allen's patches to correctly handle 32bit int/64bit ptr
sizes is going to require a bump to APR version 2.0 for the API changes.
So folding these changes into a later version of APR should prove fairly
harmless.
Bill
Re: [PATCH] add ap_brigade_insert_file()
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Saturday, November 6, 2004 12:38 PM -0700 Paul Querna
<ch...@force-elite.com> wrote:
> Why couldn't it be in an APU-1.0.1 release?
<http://apr.apache.org/versioning.html>
We can only add functions when we bump the minor revision. And, we can't
remove any function or modify its signature until we bump major. -- justin
Re: [PATCH] add ap_brigade_insert_file()
Posted by Paul Querna <ch...@force-elite.com>.
Cliff Woolley wrote:
> On Sat, 6 Nov 2004, Justin Erenkrantz wrote:
>
>
>>Point to note that if it goes into APR, it'd have to be in APR-util 1.1. I'd
>>be fine with making httpd depend on 1.1+, but...
>>
>>So, perhaps httpd is 'right' after all. -- justin
>
>
> Well, there's no reason it can't go into apu 1.1 and be "backported" to
> httpd in the meanwhile, if you take my meaning.
>
Why couldn't it be in an APU-1.0.1 release?
Re: [PATCH] add ap_brigade_insert_file()
Posted by Cliff Woolley <jw...@virginia.edu>.
On Sat, 6 Nov 2004, Justin Erenkrantz wrote:
> Point to note that if it goes into APR, it'd have to be in APR-util 1.1. I'd
> be fine with making httpd depend on 1.1+, but...
>
> So, perhaps httpd is 'right' after all. -- justin
Well, there's no reason it can't go into apu 1.1 and be "backported" to
httpd in the meanwhile, if you take my meaning.
Re: [PATCH] add ap_brigade_insert_file()
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, November 4, 2004 10:34 PM -0800 Justin Erenkrantz
<ju...@erenkrantz.com> wrote:
>> Any comments, naming preferences, "put it in apr-util you twit" flames?
>
> "Put it in apr-util, you marvelous angel." Pretty please with sugar on
> top...
Point to note that if it goes into APR, it'd have to be in APR-util 1.1. I'd
be fine with making httpd depend on 1.1+, but...
So, perhaps httpd is 'right' after all. -- justin
Re: [PATCH] add ap_brigade_insert_file()
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, November 4, 2004 10:07 PM +0000 Joe Orton <jo...@redhat.com>
wrote:
> This factors out the logic for correctly inserting a file into a
> brigade, doing the special little dance for large files if necessary;
> the code was already duplicated twice and it's needed in other places
> too. (pervasive largefile support being my top 2.1 goal :)
*nod*
> Any comments, naming preferences, "put it in apr-util you twit" flames?
"Put it in apr-util, you marvelous angel." Pretty please with sugar on top...
Looks fine otherwise... =) -- justin