Now that we're actually using this function, I figured it was time to dig in
and spend the time with this patch. :-)
On Tue, Dec 12, 2000 at 09:11:07AM -0800, Cliff Woolley wrote:
> --- Greg Stein <[EMAIL PROTECTED]> wrote:
> > This patch is much better, and I've gone ahead and applied it. However,
> > you're discarding the return values from ap_bucket_read(). Those should be
> > returned. The prototype ought to look like:
> >
> > apr_status_t ap_brigade_partition(ap_bucket_brigade *b, apr_off_t point,
> > ap_bucket **after_point);
>
> Okay. It was returning a pointer because it was trying to match
> ap_bucket_split() back
> in the days when it was _split_offset() returning a brigade. I have no
> problem with it
> returning an apr_status_t now that it has a life of its own. Patch included.
Great.
>...
> Besides changing the return values, one thing this patch does is to ditch the
> checks for
> AP_BUCKET_IS_EOS, because they're kind of superfluous... if
> point==brigade_len,
> after_point will be the EOS bucket anyway, so as long as there's never any
> data after the
> EOS, this function doesn't need to check for EOS. That lets us collapse some
> of the
> if/elseif/else stuff and have less code duplication.
Ah. Good point.
> As far as after_point goes, I've assumed in this patch that after_point need
> only be set
> in success cases... does that jive?
Yup.
Some comments on the 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.
Didn't we have another function? A copy() function or something? What
happened to that one? (or should I troll my mail archive)
Cheers,
-g
> --- src/buckets/ap_buckets.c 2000/12/12 12:18:46 1.37
> +++ src/buckets/ap_buckets.c 2000/12/12 17:08:56
> @@ -122,48 +122,44 @@
> return a;
> }
>
> -APR_DECLARE(ap_bucket *) ap_brigade_partition(ap_bucket_brigade *b,
> apr_off_t point)
> +APR_DECLARE(apr_status_t) ap_brigade_partition(ap_bucket_brigade *b,
> + apr_off_t point,
> + ap_bucket **after_point)
> {
> ap_bucket *e;
> const char *s;
> apr_size_t len;
> + apr_status_t rv;
>
> if (point < 0)
> - return NULL;
> + return APR_EINVAL;
>
> AP_BRIGADE_FOREACH(e, b) {
> /* bucket is of a known length */
> - if ((point > e->length) && (e->length != -1)) {
> - if (AP_BUCKET_IS_EOS(e))
> - return NULL;
> - point -= e->length;
> - }
> - else if (point == e->length) {
> - return AP_BUCKET_NEXT(e);
> - }
> - else {
> + if (point < e->length) {
> /* try to split the bucket natively */
> - if (ap_bucket_split(e, point) != APR_ENOTIMPL)
> - return AP_BUCKET_NEXT(e);
> + if ((rv = ap_bucket_split(e, point)) != APR_ENOTIMPL) {
> + *after_point = AP_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 (ap_bucket_read(e, &s, &len, AP_BLOCK_READ) != APR_SUCCESS)
> - return NULL;
> + if ((rv=ap_bucket_read(e, &s, &len, AP_BLOCK_READ)) !=
> APR_SUCCESS)
> + return rv;
>
> if (point < len) {
> - if (ap_bucket_split(e, point) == APR_SUCCESS)
> - return AP_BUCKET_NEXT(e);
> - else
> - return NULL;
> + *after_point = AP_BUCKET_NEXT(e);
> + return ap_bucket_split(e, point);
> }
> - else if (point == len)
> - return AP_BUCKET_NEXT(e);
> - else
> - point -= len;
> }
> + if (point == e->length) {
> + *after_point = AP_BUCKET_NEXT(e);
> + return APR_SUCCESS;
> + }
> + point -= e->length;
> }
> - return NULL;
> + return APR_EINVAL;
> }
>
> APR_DECLARE(int) ap_brigade_to_iovec(ap_bucket_brigade *b,
--
Greg Stein, http://www.lyra.org/
