[PATCH] Update mail parsing to be per protocol (imap/pop/smtp)

[Kunal Pariani]

Some comments received on the patch from Filipe

I can add that I think your patch is valuable.
But half of the changes are not related to the main patch goals:
- expected args count check
- additional error messages.

Just a quick review of your patch:
- it is too long to be easily reviewed.

- please keep the blank lines from original code. ( lines with just a
- remove them ).

- moving and grouping of enum has no added value from my point of view.
Because of the ngx_mail_set_<proto>_parse_state_start() and
ngx_mail_set_imap_parse_state_argument() methods, you need to push the
enums declarations out of parse() methods.

=> use a macro to replace the ngx_mail_set_<proto>_parse_state_start method.
=> not sure that a macro will be compliant with nginx standard.

- ngx_mail_set_imap_parse_state_start() and co. has no value added,
unless you want to put a breakpoint to catch these step.
- same about ngx_mail_set_imap_parse_state_argument() and co.


Can i know the possibility of this patch being considered if i make the above suggested changes or any others if there ?

Thanks
-Kunal
----- Original Message -----
From: "Kunal Pariani" <kpariani at zimbra.com>
To: "nginx-devel" <nginx-devel at nginx.org>
Sent: Thursday, September 18, 2014 10:32:48 AM
Subject: Re: [PATCH] Update mail parsing to be per protocol (imap/pop/smtp)

The patch certainly adds more intelligence in the mail parser per protocol (adding more specific states & error codes) and seems valid even with the latest nginx mainline. 

Thanks
-Kunal

----- Original Message -----
From: "Maxim Dounin" <mdounin at mdounin.ru>
To: "nginx-devel" <nginx-devel at nginx.org>
Sent: Thursday, September 18, 2014 4:00:32 AM
Subject: Re: [PATCH] Update mail parsing to be per protocol (imap/pop/smtp)

Hello!

On Wed, Sep 17, 2014 at 03:49:50PM -0500, Kunal Pariani wrote:

> Hello,
> This patch adds separate mail parsing states per protocol 
> (imap/pop/smtp), specific error codes for parsing/auth/login 
> failures. Also includes some minor bug fixes in the mail parsing 
> code. Requesting to include this patch as Zimbra has been using 
> this since nginx 0.5.37, so it's quite heavily tested.

Mail parsing code a bit changed since 0.5.37, so this is more like 
an argument against.

In any case, it's not clear what the patch tries to achieve.  It's 
highly unlikely that it will be considered, see
http://nginx.org/en/docs/contributing_changes.html.

-- 
Maxim Dounin
http://nginx.org/

_______________________________________________
nginx-devel mailing list
nginx-devel at nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

_______________________________________________
nginx-devel mailing list
nginx-devel at nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel