On 11 Nov 2011, at 03:33, Joe Orton wrote: Feel free to fix issues you find. That's the advantage of having it under change control @apache.org.
> a) gcc warnings: Indeed, checking those return values would be better. May have been lost when I separated out the i18n code from its origins in markup filtering. > b) code style => http://httpd.apache.org/dev/styleguide.html Yep. I made the more important changes - like tab removal - but there are limits to what one can do in a session. > d) This part which is supposed to cope with a brigade of non-determinate > length doesn't look right - such a brigade is likely to contain a bucket > type for which ->setaside will fail, so, you can't expect it will > succeed: > > /* not enough data to sniff. Wait for more */ > APR_BRIGADE_DO(b, ctx->bbsave) { > apr_bucket_setaside(b, f->r->pool); > } What are you suggesting would be likely to feed it buckets with not just no setaside but also insufficient data to sniff? The primary use case I've considered is mod_proxy, but maybe something like PHP might look uglier. It's already an edge-case that that code should ever get invoked. How would you handle it without setaside? Test the return value and give up trying to sniff on error? > ap_fwrite(f->next, ctx->bbnext, ctx->buf, > (apr_size_t)ctx->bblen - ctx->bytes); Feel free to fill the gaps! > f) dubious cast: > > rv = apr_bucket_read(b, (const char**)&buf, &bytes, > APR_BLOCK_READ); > > the returned pointer from ->read is const for a reason; e.g. for an > IMMORTAL bucket, it will be in unwritable memory; this code seems to > assume it is writable. It is treated as writable in the if/else alternative path to filling buf with data (in apr_brigade_flatten). That's mutually exclusive with the above line. So I guess the issue is the signature of xml2enc_run_preprocess. -- Nick Kew
