Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Content-Type: text/plain;
  charset="iso-8859-1"
From: Ryan Bloom <rbb@covalent.net>
Reply-To: rbb@covalent.net
Organization: Covalent Technologies
To: Justin Erenkrantz <jerenkrantz@ebuilt.com>
Subject: Re: [PATCH] Rework httpd buckets/filtering
Date: Sun, 23 Sep 2001 13:05:17 -0700
Cc: dev@httpd.apache.org
References: <20010923041203.H6756@ebuilt.com>
 <20010923153307.AFA6346DFC@koj.rkbloom.net> <20010923094847.I6756@ebuilt.com>
In-Reply-To: <20010923094847.I6756@ebuilt.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Message-Id: <20010923200518.51B0046DFC@koj.rkbloom.net>

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 **)&current, &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				rbb@apache.org
Covalent Technologies			rbb@covalent.net
--------------------------------------------------------------