Hello! On Mon, May 01, 2017 at 01:09:56PM +0300, Matwey V. Kornilov wrote:
> # HG changeset patch > # User Matwey V. Kornilov <[email protected]> > # 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/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
