Hello!
On Sat, Jan 28, 2017 at 02:08:29PM +0300, Matwey V. Kornilov wrote:
> # HG changeset patch
> # User Matwey V. Kornilov <matwey.korni...@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
