[PATCH] SSI: Implement #fsize SSI command

[Matwey V. Kornilov]

30.01.2017 17:26, 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.

Hello,

Thank you for valuable comments.

I probably missed something about header_only, it was tricky to 
understand how it was really supposed to be used. Probably I need other 
(or introduce new dedicated) way to achieve the same result.
Frankly speaking, I don't like idea with waited subrequest because there 
could be many #fsize-s at the same page and simultaneous #fsize 
processing would be desired here due to lower page rendering time.


>
> [...]
>
>> +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;
>