Hello!

On Wed, Oct 14, 2009 at 06:46:07AM -0400, piotreek wrote:

> Hi,
> We have written new Nginx modules and we publish them in hope of getting opinions. :) Generally, these modules are connected with XML and XSLT processing. Information and short description are available at http://xxslt.sourceforge.net
>
> I’m especially interested in review of XSLT2 module. It’s very similar to Igor Sysoev’s XSLT module, but allows to declare XSL style sheets directly in XML document and download them from external locations. Nginx events are used to download external files, so our filter have to work asynchronously. This is our first nginx module, so we had a lot of problems with that asynchronous work. If somebody is familiar with developing Nginx modules / filter, I would be grateful for any feedback, opinions, reviews, errors in code etc. This is early version, we are still learning . ;)
> I have many doubts about the finalization of request (ngx_http_xslt2_body_filter, ngx_http_xslt2_send and ngx_http_xslt2_fast_handler functions). It actually works, but I’m not sure if this is proper way to do it.
>
> Source (svn browser):
> http://xxslt.svn.sourceforge.net/viewvc/xxslt/http/modules/xslt2/ngx_http_xslt2_filter_module.c?view=markup
> Module:
> https://sourceforge.net/projects/xxslt/files/0.1.1/source/xslt2.tgz/download
>
> I will be grateful for any comments. :)

Some random comments in no particular order:

1. Posting code with inconsistent style for review may
dramatically reduce your karma.

2. Reinventing the wheel isn't really good idea, but once you
decided to reimplement http client (and not use upstream module
for this), you probably should know that gethostbyname() function
blocks and can't be used during request processing.

3. Various *_create_conf handlers should return NULL on errors, not
NGX_CONF_ERROR.

4. In 0.8.11 reference counting was introduced for request
structure instead of previously used "is connection still alive"
checks. That's why just returning NGX_DONE no longer works, as
you have to do r->main->count++ before doing this (if you are
planning to continue work with request in question). It's
probably a good idea to review this change - code suggest you
didn't get it yet.

Maxim Dounin