On Thu, Dec 11, 2014 at 3:31 PM, Graham Leggett <[email protected]> wrote:
[]
> It has always seemed wrong to me to assume what is in the brigade from an
> earlier filter (ie that the bucket will be memory resident and therefore non
> blocking).
Input filters' passed-in brigades usually have the lifetime of the
underlying connection/request.
On the client side, that's not an issue, everything is over when
c->pool is destroyed, and ctx->tmp will never be used.
On the backend side (should this filter be used that way), the proxy
module is supposed take care of the passed-out brigades' lifetime,
since f->r and f->c (hence ctx->tmp) may last shorter than the client
side, still if no one has asked for this data before they are
destroyed, they are lost and won't be used anymore either.
Ideally we should use bb->bucket_alloc (and, as request_rec, something
like bb->baton if it had existed :) ) instead of f->r and
f->c->bucket_alloc for creating ctx->bb/tmp, so that mod_proxy would
not need to setaside/transform buckets' lifetime when forwarding
responses, but that's probably another story...
>
> Incorporating the above, How about this?
Looks fine, modulo the above (didn't figure out before):
+static apr_status_t crypto_in_filter(ap_filter_t *f, apr_bucket_brigade *bb,
+ ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
+{
[]
+
+ /* if our buffer is empty, read off the network until the buffer is full */
+ if (APR_BRIGADE_EMPTY(ctx->bb)) {
[]
+
+ while (!ctx->seen_eos && ctx->remaining > 0) {
[]
+ /* if an error was received, bail out now. If the error is
+ * EAGAIN and we have not yet seen an EOS, we will definitely
+ * be called again, at which point we will send our buffered
+ * data. Instead of sending EAGAIN, some filters return an
+ * empty brigade instead when data is not yet available. In
+ * this case, we drop through and pass buffered data, if any.
+ */
+ if (APR_STATUS_IS_EAGAIN(rv)) {
+ if (APR_BRIGADE_EMPTY(ctx->bb)) {
+ return rv;
+ }
+ break;
+ }
Maybe :
if (APR_STATUS_IS_EAGAIN(rv)
|| (sv == APR_SUCCESS
&& block == APR_NONBLOCK_READ
&& APR_BRIGADE_EMPTY(ctx->tmp))) {
if (APR_BRIGADE_EMPTY(ctx->bb)) {
return rv;
}
break;
}
to be consitent with the comment.
+ if (APR_SUCCESS != rv) {
+ return rv;
+ }
+
+ for (e = APR_BRIGADE_FIRST(ctx->tmp);
+ e != APR_BRIGADE_SENTINEL(ctx->tmp);
+ e = APR_BUCKET_NEXT(e)) {
while (!APR_BRIGADE_EMPTY(ctx->tmp)) {
e = APR_BRIGADE_FIRST(ctx->tmp);
since we can't use APR_BUCKET_NEXT(e) after apr_bucket_delete(e) below.
[]
+
+ apr_bucket_delete(e);
+
+ }
+ }
+ }
+
[]
+
+ return rv;
Here we may still have APR_STATUS_IS_EAGAIN(rv), but some buffered
stuff to return, so :
return APR_SUCCESS;
+}
Regards,
Yann.
