Re: mod_xml2enc comments
On Sunday 13 November 2011, Nick Kew wrote: 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. I have added some error checks and a few ap_asserts(). Do you want to review it before I merge it into 2.4? Do you have some setup where you can test the change?
Re: mod_xml2enc comments
On Mon, 28 Nov 2011 17:31:59 +0100 Stefan Fritsch s...@sfritsch.de wrote: On Sunday 13 November 2011, Nick Kew wrote: 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. I have added some error checks and a few ap_asserts(). Do you want to review it before I merge it into 2.4? Do you have some setup where you can test the change? Great, thanks! Anything that might change behaviour outside of an error path? If all you're doing is addressing Joe's concerns (whether by design or coincidence) then please go right ahead and commit. -- Nick Kew
Re: mod_xml2enc comments
- Original Message - On Mon, 28 Nov 2011 17:31:59 +0100 Stefan Fritsch s...@sfritsch.de wrote: On Sunday 13 November 2011, Nick Kew wrote: 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. I have added some error checks and a few ap_asserts(). Do you want to review it before I merge it into 2.4? Do you have some setup where you can test the change? Great, thanks! Anything that might change behaviour outside of an error path? If all you're doing is addressing Joe's concerns (whether by design or coincidence) then please go right ahead and commit. CTR (: -- Nick Kew i -- Igor Galić Tel: +43 (0) 664 886 22 883 Mail: i.ga...@brainsware.org URL: http://brainsware.org/ GPG: 6880 4155 74BD FD7C B515 2EA5 4B1D 9E08 A097 C9AE
Re: mod_xml2enc comments
On Sun, Nov 13, 2011 at 03:42:07AM +, Nick Kew wrote: Feel free to fix issues you find. That's the advantage of having it under change control @apache.org. I don't have time/inclination, thanks. If nobody has any interest in maintaining this code, why has it been added to the tree? The ASF is not a dumping ground for dead code. Regards, Joe
Re: mod_xml2enc comments
On Wed, 23 Nov 2011 14:26:00 + Joe Orton jor...@redhat.com wrote: On Sun, Nov 13, 2011 at 03:42:07AM +, Nick Kew wrote: Feel free to fix issues you find. That's the advantage of having it under change control @apache.org. I don't have time/inclination, thanks. If nobody has any interest in maintaining this code, why has it been added to the tree? The ASF is not a dumping ground for dead code. Regards, Joe Discussed last month in http://marc.info/?t=13182843771 -- Nick Kew
Re: mod_xml2enc comments
On Wed, Nov 23, 2011 at 03:38:19PM +, Nick Kew wrote: On Wed, 23 Nov 2011 14:26:00 + Joe Orton jor...@redhat.com wrote: On Sun, Nov 13, 2011 at 03:42:07AM +, Nick Kew wrote: Feel free to fix issues you find. That's the advantage of having it under change control @apache.org. I don't have time/inclination, thanks. If nobody has any interest in maintaining this code, why has it been added to the tree? The ASF is not a dumping ground for dead code. Regards, Joe Discussed last month in http://marc.info/?t=13182843771 The DIY, sucker response to code review implies there is not in fact any interest in doing collabarative development for this code at the ASF. So, again, why do we have it in the tree? Regards, Joe
Re: mod_xml2enc comments
On Wed, 23 Nov 2011 16:21:44 + Joe Orton jor...@redhat.com wrote: On Wed, Nov 23, 2011 at 03:38:19PM +, Nick Kew wrote: On Wed, 23 Nov 2011 14:26:00 + Joe Orton jor...@redhat.com wrote: On Sun, Nov 13, 2011 at 03:42:07AM +, Nick Kew wrote: Feel free to fix issues you find. That's the advantage of having it under change control @apache.org. I don't have time/inclination, thanks. If nobody has any interest in maintaining this code, why has it been added to the tree? The ASF is not a dumping ground for dead code. Regards, Joe Discussed last month in http://marc.info/?t=13182843771 The DIY, sucker The what ASF. So, again, why do we have it in the tree? Because when I suggested it in the thread referenced, there was a consensus in favour. Or do you want to deny mod_proxy_html to anyone whose pages use charsets other than ASCII or UTF-8? -- Nick Kew
Re: mod_xml2enc comments
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
mod_xml2enc comments
a) gcc warnings:
mod_xml2enc.c: In function 'fix_skipto':
mod_xml2enc.c:123:18: warning: variable 'rv' set but not used
[-Wunused-but-set-variable]
mod_xml2enc.c: In function 'sniff_encoding':
mod_xml2enc.c:167:18: warning: variable 'rv' set but not used
[-Wunused-but-set-variable]
b) code style = http://httpd.apache.org/dev/styleguide.html
e.g. request_rec *r not request_rec* r throughout
running the thing through indent might help but it will require manual
work too no doubt.
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);
}
e) no error handling in various places:
ap_fwrite(f-next, ctx-bbnext, ctx-buf,
(apr_size_t)ctx-bblen - ctx-bytes);
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.
joe
