You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2011/11/11 04:33:40 UTC
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
Re: mod_xml2enc comments
Posted by Nick Kew <ni...@webthing.com>.
On Wed, 23 Nov 2011 16:21:44 +0000
Joe Orton <jo...@redhat.com> wrote:
> On Wed, Nov 23, 2011 at 03:38:19PM +0000, Nick Kew wrote:
> > On Wed, 23 Nov 2011 14:26:00 +0000
> > Joe Orton <jo...@redhat.com> wrote:
> >
> > > On Sun, Nov 13, 2011 at 03:42:07AM +0000, 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=131828437700001
>
> 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
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Nov 23, 2011 at 03:38:19PM +0000, Nick Kew wrote:
> On Wed, 23 Nov 2011 14:26:00 +0000
> Joe Orton <jo...@redhat.com> wrote:
>
> > On Sun, Nov 13, 2011 at 03:42:07AM +0000, 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=131828437700001
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
Posted by Nick Kew <ni...@webthing.com>.
On Wed, 23 Nov 2011 14:26:00 +0000
Joe Orton <jo...@redhat.com> wrote:
> On Sun, Nov 13, 2011 at 03:42:07AM +0000, 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=131828437700001
--
Nick Kew
Re: mod_xml2enc comments
Posted by Joe Orton <jo...@redhat.com>.
On Sun, Nov 13, 2011 at 03:42:07AM +0000, 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
Posted by Igor Galić <i....@brainsware.org>.
----- Original Message -----
> On Mon, 28 Nov 2011 17:31:59 +0100
> Stefan Fritsch <sf...@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.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515 2EA5 4B1D 9E08 A097 C9AE
Re: mod_xml2enc comments
Posted by Nick Kew <ni...@webthing.com>.
On Mon, 28 Nov 2011 17:31:59 +0100
Stefan Fritsch <sf...@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
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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
Posted by Nick Kew <ni...@webthing.com>.
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