httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: appending to the content brigade
Date Fri, 24 Aug 2001 21:32:32 GMT
On Fri, Aug 24, 2001 at 04:00:46PM -0400, Cliff Woolley wrote:
> On Fri, 24 Aug 2001, Eric Prud'hommeaux wrote:
>...
> > > Note that if the last bucket is not eos, depending on how your filter is
> > > supposed to work that might be an indication that you need to stash the
> > > brigade away until you DO get an eos, because the content you're wrapping

Okay... let's back up. I believe that some poor advice may have been given.

For now, let's assume that bPayload *does* have the entire content. Given
that, there is *no* reason to copy the darn payload into a new brigade.

The whole *idea* of the filters and the brigades is that you modify the
brigade passed to you, and then pass it on down. Thus, your operation is
like this:

#define BOUNDARY "---- boundary ----"

  /* insert into head in reverse order (like pushing a stack) */
  bucket = apr_bucket_immortal_create(BOUNDARY, sizeof(BOUNDARY)-1);
  APR_BRIGADE_INSERT_HEAD(bPayload, bucket); /* boundary of data/payload */

  my_data_bucket = ...
  APR_BRIGADE_INSERT_HEAD(bPayload, my_data_bucket);

  bucket = apr_bucket_immortal_create(BOUNDARY, sizeof(BOUNDARY)-1);
  APR_BRIGADE_INSERT_HEAD(bPayload, bucket); /* first boundary */

  /* insert a boundary before the EOS bucket */
  eos = APR_BRIGADE_LAST(bPayload);
  APR_BRIGADE_INSERT_BEFORE(eos, bucket);

  return ap_pass_brigade(f->next, bPayload);


Construction of the boundary is probably dynamic, and I'd bet you have much
more data to insert. But the concept demonstrated above is how you "should"
deal with the passing brigade, and how to pass it along.

Copying the brigade is *not* the answer, if you can avoid it.

Now... the second point is that, as Cliff said, you may not get the EOS in
your brigade. Not a problem. On the *first* invocation of your filter,
insert the stuff up front and pass it along. (record the "first" in a
filter-specific context structure). When you see an EOS bucket at the end of
the brigade (using APR_BRIGADE_LAST and APR_BUCKET_IS_EOS), then you insert
the final boundary.

If your boundary is dynamically constructed, the store the string into your
context (to reuse on the various invocations of your filter). When you go to
insert it, wrap it in a POOL bucket, using whatever pool your boundary was
allocated in (to ensure the lifetimes match).

>...
> > Oops, I forgot to include the code snippet to which I was
> > referring. From ap_byterange_filter:
>...
> Hooo, that's ugly.  You don't have to do that for what you want.  The
> reason it's done here is that byteranges might overlap within the same
> request, so rather than just moving buckets to where they need to go, it
> has to make copies of all the requisite buckets for each of the ranges.

To be clearer, the client might request the same range multiple times. Thus,
we need multiple copies of the buckets.

[ and yes: we could optimize the thing to avoid copying by analyzing the
  ranges up front, determining where overlaps may occur, and only copy those
  portions; thus using the original brigade as much as possible, without any
  copies.
  
  that said: many buckets are very efficient at copying because of the whole
  refcount and sub-bucket stuff. ]

>...
> heap) and then THAT can be copied.  Actually, the block here is broken
> because it puts str into two buckets at a time, which will double-free
> str when the second bucket gets destroyed.  It should read:
> 
>     if ((rv = apr_bucket_copy(ec, &foo)) != APR_SUCCESS) {
>         if (rv == APR_ENOTIMPL) {
>             /* morph to a copyable bucket by reading */
>             apr_bucket_read(ec, &str, &len, APR_BLOCK_READ);
>             apr_bucket_copy(ec, &foo);

Or, it could simply pass 1 to heap_create to COPY the data passed. That *is*
what that parameter is for :-)

> >                 foo = apr_bucket_heap_create(str, len, 0, NULL);

becomes:

> >                 foo = apr_bucket_heap_create(str, len, 1, NULL);

>...
> That way, ec and foo end up as two apr_buckets of type heap that share a
> single apr_bucket_heap structure which points to str with a refcount of
> 2, rather than having two entirely separate apr_bucket/apr_bucket_heap
> pairs both of which point to str and each of which has a refcount of
> 1.

But that is the cooler solution :-)

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message