httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick Kew <n...@webthing.com>
Subject Re: mod_xml2enc comments
Date Sun, 13 Nov 2011 03:42:07 GMT

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

Mime
View raw message