httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@apache.org>
Subject Re: [PATCH] Switch DAV PUT to use brigades
Date Tue, 04 Jun 2002 05:56:15 GMT
On Mon, Jun 03, 2002 at 04:02:03PM -0700, Greg Stein wrote:
> > My one caveat with this is what to do when the filters return
> > an error (spec. AP_FILTER_ERROR which means that they already
> > took care of it).  In this case, the handler should NOT generate
> > an error of its own and just return the AP_FILTER_ERROR value
> > back.  mod_dav has its own error handling, so its not as
> > straightforward as the other modules.
> > 
> > Greg?  Anyone else?  -- justin
> 
> Hard to answer. What is the desired behavior? Let's attack it from that
> angle. Should it just completely exit the handler? With OK, the rv, or with
> DONE? I'm sure that we can arrange the appropriate behavior.

My idea is that the module needs to back out any intermediate
steps it performed - such as releasing a lock or other such things.
But, almost certainly, if we get AP_FILTER_ERROR returned, we need
to return that value.  If we get an APR error, then, we probably have
something to worry about.  Again, this is something that is wildly
inconsistent throughout the code.  Filters can return APR status
codes, but those don't translate to HTTP error codes.

> >...
> > +        int seen_eos;
> > +
> > +        brigade = apr_brigade_create(r->pool, r->connection->bucket_alloc);
> > +        seen_eos = 0;
> 
> Prolly easier to just set seen_eos in the decl.

Fair enough.

> >...
> > +            APR_BRIGADE_FOREACH(bucket, brigade) {
> > +                const char *data;
> > +                apr_size_t len;
> > +
> > +                if (APR_BUCKET_IS_EOS(bucket)) {
> > +                    seen_eos = 1;
> > +                    break;
> > +                }
> > +
> > +                /* Ahem, what to do? */
> > +                if (APR_BUCKET_IS_METADATA(bucket)) {
> > +                    continue;
> > +                }
> 
> No need to test for this. You'll just get zero-length buckets. If an
> important metadata bucket *does* get generated in the future, then we'd want
> to be explicitly testing for and handling it (like what is being done for
> the EOS bucket). Until then, we don't have to worry about them.

Um, I think the approach has been that you can't read from
metadata buckets.  That they may give you data back or they may
not, but they certainly shouldn't be handled if you don't know
what to do with them.  This comes out of what little I've seen
of the conversation with Cliff and Ryan.  I may have it wrong,
but I think someone said at some point that metadata may indeed
respond to bucket_read with data, but that data shouldn't be
construed as part of the input stream.  I'm thoroughly confused
at this point about what metadata buckets are.

> 
> > +                rv = apr_bucket_read(bucket, &data, &len, APR_BLOCK_READ);
> > +                if (rv != APR_SUCCESS) {
> > +                    err = dav_new_error(r->pool, HTTP_BAD_REQUEST, 0,
> > +                                        "An error occurred while reading the "
> > +                                        "request body.");
> > +                    /* This is a error which preempts us from reading to
> > +                     * EOS. */
> > +                    seen_eos = 1;
> 
> The seen_eos thing is hacky. The loop condition should watch for non-NULL
> 'err' values.

Even if an error is seen, we still need to consume the body.  So,
I don't think we can exit if err is non-NULL.  We can only exit
once we've seen the EOS.

> 
> > +                    break;
> > +                }
> > +
> > +                if (err == NULL) {
> 
> I'd recommend writing only when len!=0. No reason to call the backend
> provider with no data.

Fair enough.

> 
> > +                    /* write whatever we read, until we see an error */
> > +                    err = (*resource->hooks->write_stream)(stream,
> > +                                                           data, len);
> 
> This is missing the seen_eos (altho, per above, it shouldn't set the flag)
> and break if an error occurs. This needs to be fixed up.

Again, it's part of the idea that even if an error is seen, it should
still read until EOS.  Take a quick look at the code that is in CVS
right now as it does this.  Is this the wrong approach?  -- justin

Mime
View raw message