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/
