On Sunday 23 September 2001 09:48 am, Justin Erenkrantz wrote:
> > > Index: buckets/apr_brigade.c
> > > ===================================================================
> > > RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
> > > retrieving revision 1.25
> > > diff -u -r1.25 apr_brigade.c
> > > --- buckets/apr_brigade.c 2001/09/03 22:00:36 1.25
> > > +++ buckets/apr_brigade.c 2001/09/23 10:36:12
> > > @@ -221,6 +221,29 @@
> > > return APR_SUCCESS;
> > > }
> > >
> > > +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> > > char *c) +{
> > > + apr_bucket *e;
> > > + char *current;
> > > + apr_size_t curlen;
> > > + apr_status_t status;
> > > +
> > > + current = c;
> > > +
> > > + APR_BRIGADE_FOREACH(e, b) {
> > > +
> > > + status = apr_bucket_read(e, (const char **)¤t, &curlen,
> > > + APR_BLOCK_READ);
> >
> > This is most likely wrong. You probably want to do a non-blocking read,
> > and return immediately if you can't find any data. That way, the caller
> > can keep working with the data he has. You also probably want to split
> > the brigade at that location, so that the caller can keep track of where
> > they are in the brigade.
>
> The code that it replaced in http_protocol.c used a blocking read
> (see http_protocol.c:1563). I guess the mode could be passed in
> to this function?
Yeah. In httpd, this makes sense for the location that we are talking about.
>From a strictly library POV, this ties it too much to APR.
> > > + if (status != APR_SUCCESS)
> > > + return status;
> > > +
> > > + current += curlen;
> > > + }
> > > +
> > > + return APR_SUCCESS;
> > > +
> > > +}
> > > +
> > > APU_DECLARE(apr_status_t) apr_brigade_to_iovec(apr_bucket_brigade *b,
> > > struct iovec *vec, int
> > > *nvec) {
> > > Index: buckets/apr_buckets_pipe.c
> > > ===================================================================
> > > RCS file: /home/cvs/apr-util/buckets/apr_buckets_pipe.c,v
> > > retrieving revision 1.41
> > > diff -u -r1.41 apr_buckets_pipe.c
> > > --- buckets/apr_buckets_pipe.c 2001/09/22 22:36:07 1.41
> > > +++ buckets/apr_buckets_pipe.c 2001/09/23 10:36:13
> > > @@ -106,15 +106,8 @@
> > > }
> > > else {
> > > free(buf);
> > > - a = apr_bucket_immortal_make(a, "", 0);
> > > - *str = a->data;
> > > - if (rv == APR_EOF) {
> > > + if (rv == APR_EOF)
> > > apr_file_close(p);
> > > - if (block == APR_NONBLOCK_READ) {
> > > - /* XXX: this is bogus, should return APR_SUCCESS */
> > > - return APR_EOF;
> > > - }
> > > - }
> >
> > You can't do this. The reason for the immortal bucket, was that we are
> > closing the pipe or socket bucket. What this change does, is it closes
> > the pipe, but leaves the bucket in the brigade. This will cause problems
> > later. You can't just remove the bucket, because you only have access to
> > the one bucket, and if you remove this bucket, then you also need to
> > update the pointer from the previous bucket, which you can't do.
>
> This adds all of the 0-length buckets that make some of the higher-level
> code more complicated. Wouldn't calling apr_bucket_delete(a) work here?
> The bucket contains a link to which brigade it is in - so it should be
> able to update the right pointers, no?
See Cliff's response, he is right on.
> > > @@ -655,12 +635,16 @@
> > > if (*readbytes == -1) {
> > > apr_bucket *e;
> > > apr_off_t total;
> > > + const char *str;
> > > + apr_size_t len;
> > > APR_BRIGADE_FOREACH(e, ctx->b) {
> > > - const char *str;
> > > - apr_size_t len;
> > > + /* We don't care about these values. We just want to
> > > force the + * lower level to convert it. This seems
> > > hackish. */ apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
> >
> > Why does this seem hack-ish? We can't pass a socket up the filter stack
> > and we need to get all the data. This is the defined way to do that.
>
> It seems like we should have a function that says, "go through and
> read all the data but don't return the value to me." Actually,
> calling apr_brigade_length(ctx->b, 1, &len) would do the same
> thing - but that's even more hackish.
I would personally just use apr_brigade_length. Another function is overkill
for this IMHO.
Ryan
______________________________________________________________
Ryan Bloom [EMAIL PROTECTED]
Covalent Technologies [EMAIL PROTECTED]
--------------------------------------------------------------
