mod_layout - what is wrong?

Thu Jan 7 23:04:43 MSK 2010

Dnia czwartek 07 styczeń 2010 o 00:39:14 Maxim Dounin napisał(a):
> Hello!
> 
> On Wed, Jan 06, 2010 at 04:09:56PM +0100, witekfl Gazeta.pl wrote:
> > Hi,
> > http://rkd.republika.pl/ngx_http_layout_filter_module.c
> >
> > I want to write the mod_layout.
> > The idea is: write all chains to temp file, at the end read it, insert
> > the header after <body> and the footer before </body>.
> > Sometimes it fails. There is only the header and the footer. Why?
> > Could you review it?
> 
> Some random comments in random order:
> 
> 1. It's probably good idea to polish style a bit before posting
> code for review.

Oh sorry. nginx uses spaces, I prefer tabs.
There is mix there.

> 2. The whole idea of buffering to temporary file looks wrong.
> There is no need to buffer more than several bytes here.  And the
> only case where you need buffering is when you find something like
> "</b" at the end of buffer, and need next buffer to find out if
> it's "</body>" or "</b>".  It requires a bit more complicated
> matching code, but saves lots of resources.  See sub filter
> and ssi filter for examples.

I don't think it is so easy. If there is no <body> the header is added at the 
begining. But one doesn't know if there is no <body> unless the whole 
document is loaded.
The header is a javascript with adverts.

> 3. After writing chain to temp file you have to mark buffers in
> this chain as sent by updating buf->pos.  This will allow upper
> layers to reuse this buffers.  Note well: you probably want to
> test your code with something like
> 
>    output_buffers 1 64;
> 
> to see what happens if upper layers are out of buffers since you
> don't mark them as sent.  Hint: request will just hang awaiting
> some buffers to become free after first 64 bytes.
> 
> 4. layout_ignore_uris setting doesn't fit into nginx model of
> doing things.  One should write correct configuration using
> "location" directives instead of doing some fancy filtering at
> module level.

It is a shared hosting. nginx.conf is complicated already.

> 
> 5. You assume buffers are in memory, but do not require it.  You
> should set r->filter_need_in_memory in header filter handler.

Thanks!