Hello! On Tue, Feb 28, 2017 at 07:20:49PM +0000, Eran Kornblau wrote:
> Hi all, > > Wrote a small script to find missing 'static's in my module - > https://github.com/kaltura/nginx-vod-module/blob/master/test/test_static.py > > I executed it on nginx core and found a few of those too, see attached patch. > The logic for finding these was - > > 1. An exported symbol (found by running readelf on all object files) > > 2. Appears only in one source file (I could have checked the object > imports instead, but anyway, > that's how it works...) > > 3. Not mentioned in any header file (simple 'find whole words' search) > > 4. Does not end with '_module' > > Btw, the script also complained about ngx_noaccepting and ngx_restart, I did > not change those since > all other flags there are exported. Thank you, looks interesting. Quick review below. > # HG changeset patch > # User Eran Kornblau <eran...@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. > > 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. > @@ -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. > 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. > 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. > 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. [...] -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
