You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Justin Erenkrantz <je...@apache.org> on 2002/06/03 01:15:11 UTC
[PATCH] Switch DAV PUT to use brigades
This patch switches mod_dav to use brigades for input when
handling PUT.
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
Index: modules/dav/main/mod_dav.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/dav/main/mod_dav.c,v
retrieving revision 1.79
diff -u -r1.79 mod_dav.c
--- modules/dav/main/mod_dav.c 17 May 2002 11:33:08 -0000 1.79
+++ modules/dav/main/mod_dav.c 2 Jun 2002 23:12:53 -0000
@@ -868,10 +868,6 @@
apr_off_t range_start;
apr_off_t range_end;
- if ((result = ap_setup_client_block(r, REQUEST_CHUNKED_DECHUNK)) != OK) {
- return result;
- }
-
/* Ask repository module to resolve the resource */
err = dav_get_resource(r, 0 /* label_allowed */, 0 /* use_checked_in */,
&resource);
@@ -946,40 +942,73 @@
}
if (err == NULL) {
- if (ap_should_client_block(r)) {
- char *buffer = apr_palloc(r->pool, DAV_READ_BLOCKSIZE);
- long len;
+ apr_bucket_brigade *brigade;
+ apr_bucket *bucket;
+ apr_status_t rv;
+ int seen_eos;
+
+ brigade = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+ seen_eos = 0;
+
+ /*
+ * ### what happens if we read more/less than the amount
+ * ### specified in the Content-Range? eek...
+ */
+ do {
+ rv = ap_get_brigade(r->input_filters, brigade, AP_MODE_READBYTES,
+ APR_BLOCK_READ, DAV_READ_BLOCKSIZE);
/*
- * Once we start reading the request, then we must read the
- * whole darn thing. ap_discard_request_body() won't do anything
- * for a partially-read request.
+ * Error reading request body. This has precedence over
+ * prior errors.
*/
-
- while ((len = ap_get_client_block(r, buffer,
- DAV_READ_BLOCKSIZE)) > 0) {
- if (err == NULL) {
- /* write whatever we read, until we see an error */
- err = (*resource->hooks->write_stream)(stream,
- buffer, len);
- }
+ if (rv != APR_SUCCESS) {
+ /* If the filter didn't handle this error. */
+ if (rv != AP_FILTER_ERROR) {
+ rv = HTTP_BAD_REQUEST;
+ }
+
+ err = dav_new_error(r->pool, rv, 0,
+ "An error occurred while reading the "
+ "request body.");
+ break;
}
- /*
- * ### what happens if we read more/less than the amount
- * ### specified in the Content-Range? eek...
- */
-
- if (len == -1) {
- /*
- * Error reading request body. This has precedence over
- * prior errors.
- */
- err = dav_new_error(r->pool, HTTP_BAD_REQUEST, 0,
- "An error occurred while reading the "
- "request body.");
+ 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;
+ }
+
+ 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;
+ break;
+ }
+
+ if (err == NULL) {
+ /* write whatever we read, until we see an error */
+ err = (*resource->hooks->write_stream)(stream,
+ data, len);
+ }
}
+
+ apr_brigade_cleanup(brigade);
}
+ while (!seen_eos);
err2 = (*resource->hooks->close_stream)(stream,
err == NULL /* commit */);
Re: [PATCH] Switch DAV PUT to use brigades
Posted by Cliff Woolley <jw...@virginia.edu>.
On Mon, 3 Jun 2002, Justin Erenkrantz wrote:
> > No need to test for this. You'll just get zero-length buckets. If an
>
> 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.
Given the current implementation, APR_BUCKET_IS_METADATA(e) implies that
e->length is 0 and apr_bucket_read(e) returns "".
Skipping the read call if it's a metadata is nothing more than an
optimization in this case. (You should throw in the check for len != 0,
that's almost as good an optimization.) My feelings aren't hurt one way
or the other by having this in.
--Cliff
Re: [PATCH] Switch DAV PUT to use brigades
Posted by rb...@apache.org.
> > > + 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.
No, currently o metadata bucket responds to a read with data, and the way
the metadata flag was implemented, it isn't possible for a metadata bucket
to have data.
I don't know the code well enough to know if this is implemented
correctly, but the rule follows:
All filters must make sure that all metadata buckets that they do not
handle are sent to the next filter. If a previous filter created an error
bucket, then that bucket must be sent to the next level.
Ryan
Re: [PATCH] Switch DAV PUT to use brigades
Posted by Justin Erenkrantz <je...@apache.org>.
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
Re: [PATCH] Switch DAV PUT to use brigades
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Jun 02, 2002 at 04:15:11PM -0700, Justin Erenkrantz wrote:
> This patch switches mod_dav to use brigades for input when
> handling PUT.
Cool!
> 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.
>...
> + 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.
>...
> + 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.
> + 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.
> + break;
> + }
> +
> + if (err == NULL) {
I'd recommend writing only when len!=0. No reason to call the backend
provider with no data.
> + /* 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.
> + }
> }
> +
> + apr_brigade_cleanup(brigade);
> }
> + while (!seen_eos);
Test for an error to exit.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/