[PATCH] SSI: Implement #fsize SSI command

[Maxim Dounin]

Hello!

On Sat, Jan 28, 2017 at 02:08:29PM +0300, Matwey V. Kornilov wrote:

> # HG changeset patch
> # User Matwey V. Kornilov <matwey.kornilov at gmail.com>
> # Date 1485600652 -10800
> #      Sat Jan 28 13:50:52 2017 +0300
> # Branch fsize
> # Node ID f3bb0258beb24b975b94966a94d45e9a9f5c3c39
> # Parent  640f035293959b2d4b0ba5939d954bc517f57f77
> SSI: Implement #fsize SSI command

Thanks, looks interesting, though certainly it needs more work.  
Some comments below.

[...]

> +static ngx_int_t ngx_http_ssi_fsize(ngx_http_request_t *r,
> +    ngx_http_ssi_ctx_t *ctx, ngx_str_t **params)

Style:
The function type should be on a line by itself preceding the 
function.  That is, function name should start on its own line:

static ngx_int_t
ngx_http_ssi_fsize(ngx_http_request_t *r, ngx_http_ssi_ctx_t *ctx,
    ngx_str_t **params)

[...]

> +    psr->handler = ngx_http_ssi_fsize_output;
> +    psr->data = ctx;
> +
> +    if (ngx_http_subrequest(r, uri, &args, &sr, psr, flags) != NGX_OK) {
> +        return NGX_HTTP_SSI_ERROR;
> +    }
> +    sr->header_only = 1;
> +
> +    return NGX_OK;
> +}
> +
> +
> +static ngx_int_t ngx_http_ssi_fsize_output(ngx_http_request_t *r, void *data,
> +    ngx_int_t rc)
> +{
> +    ngx_chain_t        *out;
> +    ngx_buf_t          *b;
> +    u_char             *p;
> +    size_t              len, i;
> +    ngx_http_ssi_ctx_t *ctx;
> +    unsigned            sizefmt_bytes;
> +
> +    ctx = data;
> +    sizefmt_bytes = (ctx ? ctx->sizefmt_bytes : 0);

Just a side note:
It is not clear how ctx can be NULL here.

> +
> +    p = ngx_pnalloc(r->pool, NGX_OFF_T_LEN + 1);
> +    if (p == NULL) {
> +        return NGX_ERROR;
> +    }
> +
> +    len = ngx_sprintf(p, "%O", r->headers_out.content_length_n) -  p;

Note that r->headers_out.content_length_n may be -1 here.  Either 
because it is not known at all (this can happen for proxied 
requests), or because it was cleared/modified by a filter 
expecting to change the response.  In either case "-1" is probably 
not a correct value to print here.

It might also be a good idea to find out how to prevent filters 
from clearing the length and/or how to save it earlier.

> +
> +    if (!sizefmt_bytes) {
> +        i = len / 3;
> +        len = len % 3;
> +
> +        p[len] = ngx_http_ssi_si_prefix[i];
> +        len++;
> +    }
> +
> +    b = ngx_calloc_buf(r->pool);
> +    if (b == NULL) {
> +        return NGX_ERROR;
> +    }
> +
> +    b->memory = 1;
> +    b->pos = p;
> +    b->last = p + len;

Just a side note:
Using ngx_create_temp_buf() might be more appropriate here.

> +
> +    out= ngx_alloc_chain_link(r->pool);
> +    if (out == NULL) {
> +        return NGX_ERROR;
> +    }
> +
> +    out->buf = b;
> +    out->next = NULL;
> +
> +    return ngx_http_output_filter(r, out);

Not sure if it's ok to declare a subrequest as a header_only one, 
and then output a body for it.  I suspect it may break in 
unexpected places.

Alternative approach would be to wait for the subrequest and then 
output the result in the main request context, much like it is 
done for "include set".

> @@ -2400,6 +2529,22 @@
>          ctx->errmsg = *value;
>      }
>  
> +    value = params[NGX_HTTP_SSI_CONFIG_SIZEFMT];
> +
> +    if (value) {
> +       if (value->len == 5
> +           && ngx_strncasecmp(value->data, (u_char*)"bytes", 5) == 0) {
> +           ctx->sizefmt_bytes = 1;

Style:
The "{" should be on its own line.  And the cast needs more 
spaces,

       if (value->len == 5
           && ngx_strncasecmp(value->data, (u_char *) "bytes", 5) == 0)
       {
           ctx->sizefmt_bytes = 1;

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