Thanks for the review!
Updated patch attached, comments inline below
> Hello!
>
> > # HG changeset patch
> > # User Eran Kornblau <erankor@gmail.com> # Date 1488300547 18000
> > # Tue Feb 28 11:49:07 2017 -0500
> > # Node ID 4b4b8f5413a4a1679d6ad0aa444e29e3f55b6b2a
> > # Parent 8b7fd958c59f8280d167fe7dd93f1942bfed5876
> > add missing static specifiers
>
> Style nitpicking:
>
> Added missing static specifiers.
>
Changed
>
> >
> > diff -r 8b7fd958c59f -r 4b4b8f5413a4 src/core/ngx_resolver.c
> > --- a/src/core/ngx_resolver.c Mon Feb 27 22:36:15 2017 +0300
> > +++ b/src/core/ngx_resolver.c Tue Feb 28 11:49:07 2017 -0500
> > @@ -56,7 +56,7 @@
> > ((u_char *) (n) - offsetof(ngx_resolver_node_t, node))
> >
> >
> > -ngx_int_t ngx_udp_connect(ngx_resolver_connection_t *rec);
> > +static ngx_int_t ngx_udp_connect(ngx_resolver_connection_t *rec);
> > ngx_int_t ngx_tcp_connect(ngx_resolver_connection_t *rec);
> >
> >
>
> I'm curious why ngx_udp_connect() was catched, but exactly identical ngx_tcp_connect() wasn't.
>
Good catch! The reason is that while I applied the changes against master,
I'm usually working on old version (1.8.0) and I executed the script against
that one, this function did not exist then.
Anyway, I ran the script now against master and it did find this function,
as well as a few additional stat variables which I added now as well.
> > @@ -4379,7 +4379,7 @@
> > }
> >
> >
> > -ngx_int_t
> > +static ngx_int_t
> > ngx_udp_connect(ngx_resolver_connection_t *rec) {
> > int rc;
> > diff -r 8b7fd958c59f -r 4b4b8f5413a4 src/event/modules/ngx_epoll_module..c
> > --- a/src/event/modules/ngx_epoll_module.c Mon Feb 27 22:36:15 2017 +0300
> > +++ b/src/event/modules/ngx_epoll_module.c Tue Feb 28 11:49:07 2017 -0500
> > @@ -176,7 +176,7 @@
> > };
> >
> >
> > -ngx_event_module_t ngx_epoll_module_ctx = {
> > +static ngx_event_module_t ngx_epoll_module_ctx = {
> > &epoll_name,
> > ngx_epoll_create_conf, /* create configuration */
> > ngx_epoll_init_conf, /* init configuration */
> > diff -r 8b7fd958c59f -r 4b4b8f5413a4 src/event/ngx_event.c
> > --- a/src/event/ngx_event.c Mon Feb 27 22:36:15 2017 +0300
> > +++ b/src/event/ngx_event.c Tue Feb 28 11:49:07 2017 -0500
> > @@ -165,7 +165,7 @@
> > };
> >
> >
> > -ngx_event_module_t ngx_event_core_module_ctx = {
> > +static ngx_event_module_t ngx_event_core_module_ctx = {
> > &event_core_name,
> > ngx_event_core_create_conf, /* create configuration */
> > ngx_event_core_init_conf, /* init configuration */
>
> Note that all event modules currently use non-static contexts.
> Obviously your script can't catch symbols in modules you don't have compiled in, but a simple grep will allow to properly extend this to other modules.
>
Added
> > diff -r 8b7fd958c59f -r 4b4b8f5413a4 src/http/modules/ngx_http_charset_filter_module.c
> > --- a/src/http/modules/ngx_http_charset_filter_module.c Mon Feb 27 22:36:15 2017 +0300
> > +++ b/src/http/modules/ngx_http_charset_filter_module.c Tue Feb 28 11:49:07 2017 -0500
> > @@ -123,7 +123,7 @@
> > static ngx_int_t ngx_http_charset_postconfiguration(ngx_conf_t *cf);
> >
> >
> > -ngx_str_t ngx_http_charset_default_types[] = {
> > +static ngx_str_t ngx_http_charset_default_types[] = {
> > ngx_string("text/html"),
> > ngx_string("text/xml"),
> > ngx_string("text/plain"),
>
> The ngx_http_xslt_default_types variable seems to be exactly identical.
>
Added
> > diff -r 8b7fd958c59f -r 4b4b8f5413a4 src/http/modules/ngx_http_static_module.c
> > --- a/src/http/modules/ngx_http_static_module.c Mon Feb 27 22:36:15 2017 +0300
> > +++ b/src/http/modules/ngx_http_static_module.c Tue Feb 28 11:49:07 2017 -0500
> > @@ -14,7 +14,7 @@
> > static ngx_int_t ngx_http_static_init(ngx_conf_t *cf);
> >
> >
> > -ngx_http_module_t ngx_http_static_module_ctx = {
> > +static ngx_http_module_t ngx_http_static_module_ctx = {
> > NULL, /* preconfiguration */
> > ngx_http_static_init, /* postconfiguration */
> >
>
> As per
>
> $ grep -rh ngx_http_module_t src/ | grep -v '^static'
>
> there is also ngx_http_gzip_static_module_ctx which is also not static.
>
Added
> > diff -r 8b7fd958c59f -r 4b4b8f5413a4 src/http/ngx_http_upstream.c
> > --- a/src/http/ngx_http_upstream.c Mon Feb 27 22:36:15 2017 +0300
> > +++ b/src/http/ngx_http_upstream.c Tue Feb 28 11:49:07 2017 -0500
> > @@ -188,7 +188,7 @@
> > #endif
> >
> >
> > -ngx_http_upstream_header_t ngx_http_upstream_headers_in[] = {
> > +static ngx_http_upstream_header_t ngx_http_upstream_headers_in[] = {
> >
> > { ngx_string("Status"),
> > ngx_http_upstream_process_header_line,
> > diff -r 8b7fd958c59f -r 4b4b8f5413a4 src/os/unix/ngx_linux_init.c
> > --- a/src/os/unix/ngx_linux_init.c Mon Feb 27 22:36:15 2017 +0300
> > +++ b/src/os/unix/ngx_linux_init.c Tue Feb 28 11:49:07 2017 -0500
> > @@ -9,8 +9,8 @@
> > #include <ngx_core.h>
> >
> >
> > -u_char ngx_linux_kern_ostype[50];
> > -u_char ngx_linux_kern_osrelease[50];
> > +static u_char ngx_linux_kern_ostype[50]; static u_char
> > +ngx_linux_kern_osrelease[50];
>
> There are various OS-specific variables for various other platforms as well. It would be a good idea to either review them all, or left them as is.
>
Only one I could find is SERVICE_TABLE_ENTRY st (went over win32 files manually)
Added it
> [...]
>
> --
> Maxim Dounin
> http://nginx.org/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
Eran
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
