apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: [PATCH] ap_brigade_partition()
Date Wed, 24 Jan 2001 07:40:59 GMT
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 <gstein@lyra.org> 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/

Mime
View raw message