--- Greg Stein <[EMAIL PROTECTED]> wrote:
> Now that we're actually using this function, I figured it was time to dig in
> and spend the time with this patch. :-)
> *) what happens when you find a bucket with e->length == -1 ? AFAICT, the
> function doesn't handle it. I'm thinking a check at the top of the loop
> for a length==-1 and a read(), then do the point/length compare stuff.
>
> *) after_point looks wrong for the manual-split case. It appears that you'd
> end up with (B1a, B1b, B2) and return B2 rather than B1b.
Both of these problems should now be taken care of. My bad. =-)
One note: I changed a conditional from (point < len) where le is returned from
apr_bucket_read() to (point < e->length) to match a later test for (point ==
e->length).
I added a comment explaining why this is okay (namely that len == e->length).
In every
currently existing bucket type, if read wants to return some data that is less
than what
the bucket contains, it always morphs the bucket into two buckets and adjusts
the
e->length values. I also am 99% sure that Ryan told me at one point that the
current
design doesn't allow for partial bucket reads anyway, which makes sense given
what I've
seen. I guess I really don't understand the point of the len parameter to
apr_bucket_read(), since the value it returns is easily fetched from e->length
at any
time. Most bucket types compute it from end - start as opposed to just using
e->length,
but the two should be equal (if they're not, it's a bug AFAICT).
I also added an "outline" for the changes necessary to the byterange filter in
Apache
(which is why I copied this message to new-httpd)... but all it does is make
the filter
compile with the new API, not do anything useful with the new information. The
part I
left out is that I don't know what the correct way to report an error from
apr_brigade_partition is in terms for handling byteranges.
> Didn't we have another function? A copy() function or something? What
> happened to that one? (or should I troll my mail archive)
Uhm... apr_brigade_partition has gone through many phases, of course...
split_any, then
brigade_split, and now brigade_partition. But AFAIK, it's the only remaining
function in
question. I mean, we added the bucket copy functions a while back, if that's
what you're
thinking of... other than that, I have no idea what you're talking about. ;-)
I'm attaching the patch rather than inlining it because it contains some long
lines that
would wrap with my mail client. Hope that doesn't cause anybody too much grief.
--Cliff
__________________________________________________
Do You Yahoo!?
Yahoo! Auctions - Buy the things you want at great prices.
http://auctions.yahoo.com/
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.287
diff -u -r1.287 http_protocol.c
--- modules/http/http_protocol.c 2001/01/28 04:07:03 1.287
+++ modules/http/http_protocol.c 2001/01/29 04:25:53
@@ -227,7 +227,7 @@
char *current;
char *bound_head;
int clength = 0;
- apr_status_t rv;
+ apr_status_t rv, rv2;
int found = 0;
if (!ctx) {
@@ -328,8 +328,11 @@
APR_BRIGADE_INSERT_TAIL(bsend, e);
}
- e = apr_brigade_partition(bb, range_start);
- e2 = apr_brigade_partition(bb, range_end + 1);
+ rv = apr_brigade_partition(bb, range_start, &e);
+ rv2 = apr_brigade_partition(bb, range_end + 1, &e2);
+ if ((rv != APR_SUCCESS) || (rv2 != APR_SUCCESS)) {
+ /* XXX: what is the correct action here??? */
+ }
ec = e;
do {
Index: srclib/apr-util/buckets/apr_brigade.c
===================================================================
RCS file: /home/cvspublic/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.3
diff -u -r1.3 apr_brigade.c
--- srclib/apr-util/buckets/apr_brigade.c 2001/01/19 07:01:59 1.3
+++ srclib/apr-util/buckets/apr_brigade.c 2001/01/29 04:26:13
@@ -123,48 +123,47 @@
return a;
}
-APU_DECLARE(apr_bucket *) apr_brigade_partition(apr_bucket_brigade *b,
apr_off_t point)
+APU_DECLARE(apr_status_t) apr_brigade_partition(apr_bucket_brigade *b,
+ apr_off_t point,
+ apr_bucket **after_point)
{
apr_bucket *e;
const char *s;
apr_size_t len;
+ apr_status_t rv;
if (point < 0)
- return NULL;
+ return APR_EINVAL;
APR_BRIGADE_FOREACH(e, b) {
- /* bucket is of a known length */
- if ((point > e->length) && (e->length != -1)) {
- if (APR_BUCKET_IS_EOS(e))
- return NULL;
- point -= e->length;
- }
- else if (point == e->length) {
- return APR_BUCKET_NEXT(e);
- }
- else {
+ if ((point < e->length) || (e->length == -1)) {
/* try to split the bucket natively */
- if (apr_bucket_split(e, point) != APR_ENOTIMPL)
- return APR_BUCKET_NEXT(e);
+ if ((rv = apr_bucket_split(e, point)) != APR_ENOTIMPL) {
+ *after_point = APR_BUCKET_NEXT(e);
+ return rv;
+ }
/* if the bucket cannot be split, we must read from it,
* changing its type to one that can be split */
- if (apr_bucket_read(e, &s, &len, APR_BLOCK_READ) != APR_SUCCESS)
- return NULL;
+ if ((rv=apr_bucket_read(e,&s,&len,APR_BLOCK_READ)) != APR_SUCCESS)
+ return rv;
- if (point < len) {
- if (apr_bucket_split(e, point) == APR_SUCCESS)
- return APR_BUCKET_NEXT(e);
- else
- return NULL;
+ /* this assumes that len == e->length, which is okay because e
+ * might have been morphed by the apr_bucket_read() above, but
+ * if it was, the length would have been adjusted appropriately */
+ if (point < e->length) {
+ rv = apr_bucket_split(e, point);
+ *after_point = APR_BUCKET_NEXT(e);
+ return rv;
}
- else if (point == len)
- return APR_BUCKET_NEXT(e);
- else
- point -= len;
}
+ if (point == e->length) {
+ *after_point = APR_BUCKET_NEXT(e);
+ return APR_SUCCESS;
+ }
+ point -= e->length;
}
- return NULL;
+ return APR_EINVAL;
}
APU_DECLARE(int) apr_brigade_to_iovec(apr_bucket_brigade *b,
Index: srclib/apr-util/include/apr_buckets.h
===================================================================
RCS file: /home/cvspublic/apr-util/include/apr_buckets.h,v
retrieving revision 1.68
diff -u -r1.68 apr_buckets.h
--- srclib/apr-util/include/apr_buckets.h 2001/01/27 14:52:55 1.68
+++ srclib/apr-util/include/apr_buckets.h 2001/01/29 04:26:38
@@ -598,11 +598,12 @@
* of bytes from the brigade; the ranges can even overlap.
* @param b The brigade to partition
* @param point The offset at which to partition the brigade
- * @return A pointer to the first bucket after the partition;
- * or NULL in any error condition (including partition past the end)
- * @deffunc apr_bucket *apr_brigade_partition(apr_bucket_brigade *b, apr_off_t
point)
+ * @param after_point Returns a pointer to the first bucket after the partition
+ * @deffunc apr_status_t apr_brigade_partition(apr_bucket_brigade *b,
apr_off_t point, apr_bucket **after_point)
*/
-APU_DECLARE(apr_bucket *) apr_brigade_partition(apr_bucket_brigade *b,
apr_off_t point);
+APU_DECLARE(apr_status_t) apr_brigade_partition(apr_bucket_brigade *b,
+ apr_off_t point,
+ apr_bucket **after_point);
#if APR_NOT_DONE_YET
/**
