Hello!

On Sun, Jul 12, 2015 at 08:51:36AM -0700, Kurtis Nusbaum wrote:

In general I agree with the change, please see below for some
nitpicking.

> # HG changeset patch
> # User Kurtis Nusbaum <klnusbaum@gmail.com>
> # Date 1436715098 25200
> # Sun Jul 12 08:31:38 2015 -0700
> # Node ID 8d31439f186889335c5fd6d14be70c55e5b99fbc
> # Parent dcae651b2a0cbd3de2f1fd5cf5b8c72627db94fd
> Extract out version info function

Trailing dot, please.

>
> The code for displaying version info and configuration info seemed to be cluttering up the
> main function. I was finding it hard to read main. This extracts out all of the logic
> for displaying version and configuration info into its own function, thus making
> main easier to read.

No more than 80 chars, please.

>
> diff -r dcae651b2a0c -r 8d31439f1868 src/core/nginx.c
> --- a/src/core/nginx.c Tue Jul 07 16:38:49 2015 +0300
> +++ b/src/core/nginx.c Sun Jul 12 08:31:38 2015 -0700
> @@ -9,7 +9,7 @@
> #include <ngx_core.h>
> #include <nginx.h>
>
> -
> +static void ngx_display_version_info();
> static ngx_int_t ngx_add_inherited_sockets(ngx_cycle_t *cycle);
> static ngx_int_t ngx_get_options(int argc, char *const *argv);
> static ngx_int_t ngx_process_options(ngx_cycle_t *cycle);

Two empty strings are intentional, see
http://nginx.org/en/docs/contributing_changes.html.

The "ngx_show_version_info()" name will probably be a bit better,
as it's more in line with the "ngx_show_version" name used for the
corresponding variable.

> @@ -194,68 +194,11 @@
> }
>
> if (ngx_show_version) {
> - ngx_write_stderr("nginx version: " NGINX_VER_BUILD NGX_LINEFEED);
> + ngx_display_version_info();
> + }
>
> - if (ngx_show_help) {
> - ngx_write_stderr(
> - "Usage: nginx [-?hvVtTq] [-s signal] [-c filename] "
> - "[-p prefix] [-g directives]" NGX_LINEFEED
> - NGX_LINEFEED
> - "Options:" NGX_LINEFEED
> - " -?,-h : this help" NGX_LINEFEED
> - " -v : show version and exit" NGX_LINEFEED
> - " -V : show version and configure options then exit"
> - NGX_LINEFEED
> - " -t : test configuration and exit" NGX_LINEFEED
> - " -T : test configuration, dump it and exit"
> - NGX_LINEFEED
> - " -q : suppress non-error messages "
> - "during configuration testing" NGX_LINEFEED
> - " -s signal : send signal to a master process: "
> - "stop, quit, reopen, reload" NGX_LINEFEED
> -#ifdef NGX_PREFIX
> - " -p prefix : set prefix path (default: "
> - NGX_PREFIX ")" NGX_LINEFEED
> -#else
> - " -p prefix : set prefix path (default: NONE)" NGX_LINEFEED
> -#endif
> - " -c filename : set configuration file (default: "
> - NGX_CONF_PATH ")" NGX_LINEFEED
> - " -g directives : set global directives out of configuration "
> - "file" NGX_LINEFEED NGX_LINEFEED
> - );
> - }
> -
> - if (ngx_show_configure) {
> -
> -#ifdef NGX_COMPILER
> - ngx_write_stderr("built by " NGX_COMPILER NGX_LINEFEED);
> -#endif
> -
> -#if (NGX_SSL)
> - if (SSLeay() == SSLEAY_VERSION_NUMBER) {
> - ngx_write_stderr("built with " OPENSSL_VERSION_TEXT
> - NGX_LINEFEED);
> - } else {
> - ngx_write_stderr("built with " OPENSSL_VERSION_TEXT
> - " (running with ");
> - ngx_write_stderr((char *) (uintptr_t)
> - SSLeay_version(SSLEAY_VERSION));
> - ngx_write_stderr(")" NGX_LINEFEED);
> - }
> -#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
> - ngx_write_stderr("TLS SNI support enabled" NGX_LINEFEED);
> -#else
> - ngx_write_stderr("TLS SNI support disabled" NGX_LINEFEED);
> -#endif
> -#endif
> -
> - ngx_write_stderr("configure arguments:" NGX_CONFIGURE NGX_LINEFEED);
> - }
> -
> - if (!ngx_test_config) {
> - return 0;
> - }
> + if (ngx_show_version && !ngx_test_config) {
> + return 0;
> }

I think that separate if() here isn't a good move, it would be
better to keep it the "if (!ngx_test_config)" test inside "if
(ngx_show_version)".

>
> /* TODO */ ngx_max_sockets = -1;
> @@ -418,6 +361,68 @@
> return 0;
> }
>
> +static void
> +ngx_display_version_info()

There should be two empty lines between functions.

> +{
> + ngx_write_stderr("nginx version: " NGINX_VER_BUILD NGX_LINEFEED);
> +
> + if (ngx_show_help) {
> + ngx_write_stderr(
> + "Usage: nginx [-?hvVtTq] [-s signal] [-c filename] "
> + "[-p prefix] [-g directives]" NGX_LINEFEED
> + NGX_LINEFEED
> + "Options:" NGX_LINEFEED
> + " -?,-h : this help" NGX_LINEFEED
> + " -v : show version and exit" NGX_LINEFEED
> + " -V : show version and configure options then exit"
> + NGX_LINEFEED
> + " -t : test configuration and exit" NGX_LINEFEED
> + " -T : test configuration, dump it and exit"
> + NGX_LINEFEED
> + " -q : suppress non-error messages "
> + "during configuration testing" NGX_LINEFEED
> + " -s signal : send signal to a master process: "
> + "stop, quit, reopen, reload" NGX_LINEFEED
> +#ifdef NGX_PREFIX
> + " -p prefix : set prefix path (default: "
> + NGX_PREFIX ")" NGX_LINEFEED
> +#else
> + " -p prefix : set prefix path (default: NONE)" NGX_LINEFEED
> +#endif
> + " -c filename : set configuration file (default: "
> + NGX_CONF_PATH ")" NGX_LINEFEED
> + " -g directives : set global directives out of configuration "
> + "file" NGX_LINEFEED NGX_LINEFEED
> + );
> + }
> +
> + if (ngx_show_configure) {
> +
> +#ifdef NGX_COMPILER
> + ngx_write_stderr("built by " NGX_COMPILER NGX_LINEFEED);
> +#endif
> +
> +#if (NGX_SSL)
> + if (SSLeay() == SSLEAY_VERSION_NUMBER) {
> + ngx_write_stderr("built with " OPENSSL_VERSION_TEXT
> + NGX_LINEFEED);
> + } else {
> + ngx_write_stderr("built with " OPENSSL_VERSION_TEXT
> + " (running with ");
> + ngx_write_stderr((char *) (uintptr_t)
> + SSLeay_version(SSLEAY_VERSION));
> + ngx_write_stderr(")" NGX_LINEFEED);
> + }
> +#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
> + ngx_write_stderr("TLS SNI support enabled" NGX_LINEFEED);
> +#else
> + ngx_write_stderr("TLS SNI support disabled" NGX_LINEFEED);
> +#endif
> +#endif
> +
> + ngx_write_stderr("configure arguments:" NGX_CONFIGURE NGX_LINEFEED);
> + }

Some of the lines above need to be adjusted to the new
indentation, in some cases line wrapping is no longer needed.

> +}
>
> static ngx_int_t
> ngx_add_inherited_sockets(ngx_cycle_t *cycle)

Again, two empty lines.

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

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