[PATCH 2 of 3] SSI: Refactor ngx_http_ssi_date_gmt_local_variable

[Maxim Dounin]

Hello!

On Fri, Mar 10, 2017 at 12:10:33PM +0300, Matwey V. Kornilov wrote:

> # HG changeset patch
> # User Matwey V. Kornilov <matwey.kornilov at gmail.com>
> # Date 1486576729 -10800
> #      Wed Feb 08 20:58:49 2017 +0300
> # Branch fsize
> # Node ID 35527b6bd4d84e41e030972b770e75ef583cf0f5
> # Parent  a682e88cdcddb04905814cdfacde3df9ebc2b48b
> SSI: Refactor ngx_http_ssi_date_gmt_local_variable

Style, should be:

SSI: refactored ngx_http_ssi_date_gmt_local_variable().

> 
> Introduce new function ngx_http_ssi_format_time.
> The function will be reused in the following commit.
> 
> diff -r a682e88cdcdd -r 35527b6bd4d8 src/http/modules/ngx_http_ssi_filter_module.c
> --- a/src/http/modules/ngx_http_ssi_filter_module.c	Fri Mar 10 12:02:53 2017 +0300
> +++ b/src/http/modules/ngx_http_ssi_filter_module.c	Wed Feb 08 20:58:49 2017 +0300
> @@ -110,6 +110,8 @@
>  static ngx_int_t ngx_http_ssi_endblock(ngx_http_request_t *r,
>      ngx_http_ssi_ctx_t *ctx, ngx_str_t **params);
>  
> +static ngx_int_t ngx_http_ssi_format_time(ngx_http_request_t *r, ngx_str_t *v,
> +    time_t time, ngx_str_t *timefmt, uintptr_t gmt);
>  static ngx_int_t ngx_http_ssi_date_gmt_local_variable(ngx_http_request_t *r,
>      ngx_http_variable_value_t *v, uintptr_t gmt);
>  

General style rule is to place calling functions above the 
functions they call.  So it should be 
ngx_http_ssi_date_gmt_local_variable() and then 
ngx_http_ssi_format_time().  The same applies for both 
declarations and definitions.

> @@ -2874,25 +2876,12 @@
>  
>  
>  static ngx_int_t
> -ngx_http_ssi_date_gmt_local_variable(ngx_http_request_t *r,
> -    ngx_http_variable_value_t *v, uintptr_t gmt)
> +ngx_http_ssi_format_time(ngx_http_request_t *r, ngx_str_t *v, time_t time,
> +    ngx_str_t *timefmt, uintptr_t gmt)
>  {
> -    time_t               now;
> -    ngx_http_ssi_ctx_t  *ctx;
> -    ngx_str_t           *timefmt;
>      struct tm            tm;
>      char                 buf[NGX_HTTP_SSI_DATE_LEN];
>  
> -    v->valid = 1;
> -    v->no_cacheable = 0;
> -    v->not_found = 0;
> -
> -    now = ngx_time();
> -
> -    ctx = ngx_http_get_module_ctx(r, ngx_http_ssi_filter_module);
> -
> -    timefmt = ctx ? &ctx->timefmt : &ngx_http_ssi_timefmt;
> -
>      if (timefmt->len == sizeof("%s") - 1
>          && timefmt->data[0] == '%' && timefmt->data[1] == 's')
>      {
> @@ -2901,15 +2890,15 @@
>              return NGX_ERROR;
>          }
>  
> -        v->len = ngx_sprintf(v->data, "%T", now) - v->data;
> +        v->len = ngx_sprintf(v->data, "%T", time) - v->data;
>  
>          return NGX_OK;
>      }
>  
>      if (gmt) {
> -        ngx_libc_gmtime(now, &tm);
> +        ngx_libc_gmtime(time, &tm);
>      } else {
> -        ngx_libc_localtime(now, &tm);
> +        ngx_libc_localtime(time, &tm);
>      }
>  
>      v->len = strftime(buf, NGX_HTTP_SSI_DATE_LEN,
> @@ -2930,6 +2919,36 @@
>  
>  
>  static ngx_int_t
> +ngx_http_ssi_date_gmt_local_variable(ngx_http_request_t *r,
> +    ngx_http_variable_value_t *v, uintptr_t gmt)
> +{
> +    time_t               now;
> +    ngx_http_ssi_ctx_t  *ctx;
> +    ngx_str_t           *timefmt;
> +    ngx_str_t            timestr;
> +
> +    v->valid = 1;
> +    v->no_cacheable = 0;
> +    v->not_found = 0;
> +
> +    now = ngx_time();
> +
> +    ctx = ngx_http_get_module_ctx(r, ngx_http_ssi_filter_module);
> +
> +    timefmt = ctx ? &ctx->timefmt : &ngx_http_ssi_timefmt;
> +
> +    if (ngx_http_ssi_format_time(r, &timestr, now, timefmt, gmt) != NGX_OK) {
> +        return NGX_ERROR;
> +    }
> +
> +    v->data = timestr.data;
> +    v->len  = timestr.len;
> +
> +    return NGX_OK;
> +}

Overral, I can't say I like this refactoring.  A function with 5 
complex arguments is not what we used to have, especially for such 
simple tasks.  Not to mention that ngx_str_t argument called "v" 
looks at least wierd.  If this needs changing, it should be done 
better.

Alternatively, it may be better idea to just re-implement needed 
parts in the flastmod implementation.

> +
> +
> +static ngx_int_t
>  ngx_http_ssi_preconfiguration(ngx_conf_t *cf)
>  {
>      ngx_int_t                  rc;

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