hello
thank you for reply and background.
> but we need someone to do the rest of the work.
Could I contribute it?
At first, I will revise this patch along your review.
On Fri, Dec 4, 2015 at 4:03 AM, Maxim Dounin <mdou...@mdounin.ru> wrote:
> Hello!
>
> On Tue, Dec 01, 2015 at 08:08:30AM +0900, junpei yoshino wrote:
>
>> # HG changeset patch
>> # User Junpei Yoshino <junpei.yosh...@gmail.com>
>> # Date 1446723407 -32400
>> # Thu Nov 05 20:36:47 2015 +0900
>> # Node ID 59cadccedf402ec325b078cb72a284465639e0fe
>> # Parent 4ccb37b04454dec6afb9476d085c06aea00adaa0
>> Http: add proxy_protocol_port variable for rfc6302
>>
>> Logging source port is recommended in rfc6302.
>> use case
>> logging
>> sending information by http request headers
>
> Proper source port logging is something nginx don't currently do
> well in various places related to addresses got from external
> sources, including realip module, X-Forwarded-For parsing,
> [ha]proxy protocol and so on.
>
> Improving port handling in various related areas is something that
> should be done, though it needs to be handled more or less
> consistently in all affected areas. Providing ports in some
> places but not others can be misleading for users. And that's why
> the patch isn't yet reviewed - sorry for the delay, but we need
> someone to do the rest of the work.
>
> Below are some comments about the patch itself.
>
>> diff -r 4ccb37b04454 -r 59cadccedf40 src/core/ngx_connection.h
>> --- a/src/core/ngx_connection.h Fri Oct 30 21:43:30 2015 +0300
>> +++ b/src/core/ngx_connection.h Thu Nov 05 20:36:47 2015 +0900
>> @@ -146,6 +146,7 @@
>> ngx_str_t addr_text;
>>
>> ngx_str_t proxy_protocol_addr;
>> + ngx_str_t proxy_protocol_port;
>
> Using a string (which takes 2 pointers) for a 16 bit port value
> seems to be excessive. It should be possible to just store a
> number instead.
>
> [...]
>
>> @@ -71,8 +71,56 @@
>> ngx_memcpy(c->proxy_protocol_addr.data, addr, len);
>> c->proxy_protocol_addr.len = len;
>>
>> + for ( ;; ) {
>> + if (p == last) {
>> + goto invalid;
>> + }
>> +
>> + ch = *p++;
>> +
>> + if (ch == ' ') {
>> + break;
>> + }
>> +
>> + if (ch != ':' && ch != '.'
>> + && (ch < 'a' || ch > 'f')
>> + && (ch < 'A' || ch > 'F')
>> + && (ch < '0' || ch > '9'))
>> + {
>> + goto invalid;
>> + }
>> + }
>
> This is probably excessive. Just using a space as a separator
> should be enough, as we aren't using the destination address.
>
> [...]
>
>> ngx_log_debug1(NGX_LOG_DEBUG_CORE, c->log, 0,
>> "PROXY protocol address: \"%V\"",
>> &c->proxy_protocol_addr);
>> + ngx_log_debug1(NGX_LOG_DEBUG_CORE, c->log, 0,
>> + "PROXY protocol port: \"%V\"", &c->proxy_protocol_port);
>
> Logging the address and the port at once should be enough.
>
> [...]
>
> --
> Maxim Dounin
> http://nginx.org/
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
--
junpei.yosh...@gmail.com
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
