[PATCH 1 of 2] SSI: implemented "fsize" SSI command
Maxim Dounin
mdounin at mdounin.ru
Fri May 19 16:25:19 UTC 2017
Hello!
On Mon, May 01, 2017 at 01:09:56PM +0300, Matwey V. Kornilov wrote:
> # HG changeset patch
> # User Matwey V. Kornilov <matwey.kornilov at gmail.com>
> # Date 1492936703 -10800
> # Sun Apr 23 11:38:23 2017 +0300
> # Branch fsize
> # Node ID 0d6c509169a32624cce431f2469b10b4f961510e
> # Parent 5116cfea1e9a84be678af10e0ff1f1fce9b00cfb
> SSI: implemented "fsize" SSI command.
In the previous review I've expressed concerns that this uses body
output in a subrequest with r->header_only set, see
http://mailman.nginx.org/pipermail/nginx-devel/2017-April/009814.html.
Do you have anything to comment on this?
[...]
> +static ngx_int_t
> +ngx_http_ssi_fsize_output(ngx_http_request_t *r, void *data, ngx_int_t rc)
> +{
> + off_t length;
> + u_char scale;
> + unsigned exact_size;
> + ngx_buf_t *b;
> + ngx_int_t size;
> + ngx_chain_t *out;
> + ngx_http_ssi_ctx_t *ctx;
Style: as I already wrote during previous review, there should be
at least two spaces between variable type and the variable.
> +
> + ctx = data;
> + exact_size = ctx->exact_size;
Note that this approach doesn't work if there is more than one
config command, as subrequests are executed in parallel and thus
this uses latest config. For example,
fsize sizefmt=bytes: <!--#config sizefmt="bytes" --><!--#fsize virtual="/8m" -->
fsize sizefmt=abbrev: <!--#config sizefmt="abbrev" --><!--#fsize virtual="/8m" -->
outputs
fsize sizefmt=bytes: 8M
fsize sizefmt=abbrev: 8M
and this certainly looks wrong (and works fine with Apache).
This might be another argument to make it blocking instead,
similar to wait="yes" in "include" command.
In the patch below I've tried to resolve this by passing current
exact_size to the subrequest directly, but it still fails to use
correct error message if it is redefined more than once in a
similar manner.
> +
> + if (r->request_output) {
> + return rc;
> + }
> +
> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> + "ssi fsize output: \"%V?%V\"", &r->uri, &r->args);
> +
> + b = ngx_create_temp_buf(r->pool, NGX_OFF_T_LEN + 2);
> + if (b == NULL) {
> + return NGX_ERROR;
> + }
> +
> + length = r->headers_out.content_length_n;
> +
> + if (!exact_size && length > 1024 * 1024 * 1024 - 1) {
> + size = (ngx_int_t) (length / (1024 * 1024 * 1024));
> + if ((length % (1024 * 1024 * 1024))
> + > (1024 * 1024 * 1024 / 2 - 1))
Style: there is no need to break the line here. Corresponding
line in the autoindex module is broken down because it doesn't fit
into 80 chars, but this is not the case here.
> + {
> + size++;
> + }
> + scale = 'G';
> +
> + } else if (!exact_size && length > 1024 * 1024 - 1) {
> + size = (ngx_int_t) (length / (1024 * 1024));
> + if ((length % (1024 * 1024)) > (1024 * 1024 / 2 - 1)) {
> + size++;
> + }
> + scale = 'M';
> +
> + } else if (!exact_size && length > 9999) {
> + size = (ngx_int_t) (length / 1024);
> + if (length % 1024 > 511) {
> + size++;
> + }
> + scale = 'K';
> +
> + } else {
> + size = (ngx_int_t) length;
> + scale = '\0';
> + }
In this code you test "exact_size" flag in each "if", and then
fallback to the last "else". IMHO, this complicates understanding
the code compared to separate handling of exact_size case as
in the autoindex module. And, more importantly, this breaks
prerequisites on using ngx_int_t type for "size", leading to
incorrect results for files larger than 2G on 32-bit platforms
when using exact_size.
> +
> + if (length < 0) {
> + b->pos = ctx->errmsg.data;
> + b->last = ctx->errmsg.data + ctx->errmsg.len;
It is not clear why size/scale where at all calculated for this
case.
It might also worth to exclude various non-200 status codes, as
currently referring to a non-existing file on a backend results in
a size of the error page returned.
Also, various strange things can happen when trying to output
anything if "rc" is NGX_ERROR. See, for example,
ngx_http_ssi_stub_output(), which doesn't try to do anything if
"rc" is NGX_ERROR, or if r->connection->error is set.
> +
> + } else if (scale) {
> + b->last = ngx_sprintf(b->last, "%6i%c", size, scale);
> +
> + } else {
> + b->last = ngx_sprintf(b->last, " %6i", size);
It is not clear why you are using the leading space here, as well
as "%6i" format. This is something important in the autoindex
module that uses fixed-width table, but I don't think this
is expected in output of the "fsize" SSI command.
> + }
> +
> + 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);
> +}
> +
> +
> +static ngx_int_t
> ngx_http_ssi_echo(ngx_http_request_t *r, ngx_http_ssi_ctx_t *ctx,
> ngx_str_t **params)
> {
> @@ -2400,6 +2561,27 @@
> 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->exact_size = 1;
> +
> + } else if (value->len == 6
> + && ngx_strncasecmp(value->data, (u_char *) "abbrev", 6) == 0)
> + {
> + ctx->exact_size = 0;
> +
> + } else {
> + ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> + "unknown size format \"%V\" "
> + "in \"config\" SSI command", value);
Style, continuation lines should be aligned with the first
function argument:
ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
"unknown size format \"%V\" "
"in \"config\" SSI command", value);
> + return NGX_HTTP_SSI_ERROR;
> + }
> + }
> +
> return NGX_OK;
> }
>
> diff -r 5116cfea1e9a -r 0d6c509169a3 src/http/modules/ngx_http_ssi_filter_module.h
> --- a/src/http/modules/ngx_http_ssi_filter_module.h Thu Apr 20 18:26:38 2017 +0300
> +++ b/src/http/modules/ngx_http_ssi_filter_module.h Sun Apr 23 11:38:23 2017 +0300
> @@ -76,6 +76,7 @@
> unsigned block:1;
> unsigned output:1;
> unsigned output_chosen:1;
> + unsigned exact_size:1;
>
> ngx_http_request_t *wait;
> void *value_buf;
Here is a patch which tries to address some of the above concerns:
diff --git a/src/http/modules/ngx_http_ssi_filter_module.c b/src/http/modules/ngx_http_ssi_filter_module.c
--- a/src/http/modules/ngx_http_ssi_filter_module.c
+++ b/src/http/modules/ngx_http_ssi_filter_module.c
@@ -2310,7 +2310,7 @@ ngx_http_ssi_fsize(ngx_http_request_t *r
}
psr->handler = ngx_http_ssi_fsize_output;
- psr->data = ctx;
+ psr->data = (void *) ctx->exact_size;
if (ngx_http_subrequest(r, uri, &args, &sr, psr, flags) != NGX_OK) {
return NGX_HTTP_SSI_ERROR;
@@ -2325,68 +2325,74 @@ ngx_http_ssi_fsize(ngx_http_request_t *r
static ngx_int_t
ngx_http_ssi_fsize_output(ngx_http_request_t *r, void *data, ngx_int_t rc)
{
- off_t length;
- u_char scale;
- unsigned exact_size;
- ngx_buf_t *b;
- ngx_int_t size;
- ngx_chain_t *out;
- ngx_http_ssi_ctx_t *ctx;
-
- ctx = data;
- exact_size = ctx->exact_size;
-
- if (r->request_output) {
+ off_t length;
+ u_char scale;
+ ngx_buf_t *b;
+ ngx_int_t size;
+ ngx_uint_t exact_size;
+ ngx_chain_t *out;
+ ngx_http_ssi_ctx_t *ctx;
+
+ if (rc == NGX_ERROR || r->connection->error || r->request_output) {
return rc;
}
ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
"ssi fsize output: \"%V?%V\"", &r->uri, &r->args);
+ exact_size = (ngx_uint_t) data;
+ ctx = ngx_http_get_module_ctx(r->parent, ngx_http_ssi_filter_module);
+
b = ngx_create_temp_buf(r->pool, NGX_OFF_T_LEN + 2);
if (b == NULL) {
return NGX_ERROR;
}
- length = r->headers_out.content_length_n;
-
- if (!exact_size && length > 1024 * 1024 * 1024 - 1) {
- size = (ngx_int_t) (length / (1024 * 1024 * 1024));
- if ((length % (1024 * 1024 * 1024))
- > (1024 * 1024 * 1024 / 2 - 1))
- {
- size++;
- }
- scale = 'G';
-
- } else if (!exact_size && length > 1024 * 1024 - 1) {
- size = (ngx_int_t) (length / (1024 * 1024));
- if ((length % (1024 * 1024)) > (1024 * 1024 / 2 - 1)) {
- size++;
- }
- scale = 'M';
-
- } else if (!exact_size && length > 9999) {
- size = (ngx_int_t) (length / 1024);
- if (length % 1024 > 511) {
- size++;
- }
- scale = 'K';
+ if (r->headers_out.status != NGX_HTTP_OK
+ || r->headers_out.content_length_n < 0)
+ {
+ b->pos = ctx->errmsg.data;
+ b->last = ctx->errmsg.data + ctx->errmsg.len;
+
+ } else if (exact_size) {
+ b->last = ngx_sprintf(b->last, "%O", r->headers_out.content_length_n);
} else {
- size = (ngx_int_t) length;
- scale = '\0';
- }
-
- if (length < 0) {
- b->pos = ctx->errmsg.data;
- b->last = ctx->errmsg.data + ctx->errmsg.len;
-
- } else if (scale) {
- b->last = ngx_sprintf(b->last, "%6i%c", size, scale);
-
- } else {
- b->last = ngx_sprintf(b->last, " %6i", size);
+ length = r->headers_out.content_length_n;
+
+ if (length > 1024 * 1024 * 1024 - 1) {
+ size = (ngx_int_t) (length / (1024 * 1024 * 1024));
+ if ((length % (1024 * 1024 * 1024)) > (1024 * 1024 * 1024 / 2 - 1))
+ {
+ size++;
+ }
+ scale = 'G';
+
+ } else if (length > 1024 * 1024 - 1) {
+ size = (ngx_int_t) (length / (1024 * 1024));
+ if ((length % (1024 * 1024)) > (1024 * 1024 / 2 - 1)) {
+ size++;
+ }
+ scale = 'M';
+
+ } else if (length > 9999) {
+ size = (ngx_int_t) (length / 1024);
+ if (length % 1024 > 511) {
+ size++;
+ }
+ scale = 'K';
+
+ } else {
+ size = (ngx_int_t) length;
+ scale = '\0';
+ }
+
+ if (scale) {
+ b->last = ngx_sprintf(b->last, "%i%c", size, scale);
+
+ } else {
+ b->last = ngx_sprintf(b->last, "%i", size);
+ }
}
out = ngx_alloc_chain_link(r->pool);
@@ -2576,8 +2582,8 @@ ngx_http_ssi_config(ngx_http_request_t *
} else {
ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
- "unknown size format \"%V\" "
- "in \"config\" SSI command", value);
+ "unknown size format \"%V\" "
+ "in \"config\" SSI command", value);
return NGX_HTTP_SSI_ERROR;
}
}
--
Maxim Dounin
http://nginx.org/
More information about the nginx-devel
mailing list