Thanks Maxim, comments inline.

> Hello!
>
> On Mon, Aug 07, 2017 at 10:36:19AM +0000, Eran Kornblau wrote:
>
> Just a quick note: for me, the whole feature looks questionable, and the implementation is far from being in a commitable state.
>
Why questionable? you would probably agree that variables in the access log name can be useful in some cases,
and you'd probably also agree that log compression is good (it saves a lot of IO for a negligible amount of CPU -
we use it in all nginx servers we have). So why not allow both?
Regarding the implementation, I tried to match the coding standard (80 chars width, braces etc.) but looks like
I missed... I will happily apply any changes you request.

> > Btw, on a similar subject - can anyone explain the purpose of checking
> > the existence of the root dir in scripted logs? Only explanation I
> > could think of is to provide some security protection in case $uri is used in the script, but that sounds like a very specific use case to me...
> > Anyway, if that is indeed the case, maybe it makes sense to add conf
> > directive to enable/disable this behavior?
>
> The basic purpose of this restriction is to protect configurations like
>
> server {
> access_log /spool/vhost/logs/$host;
> ...
> }
>
> from creating arbitrary log files, thus allowing DoS attacks via inode exhaustion. Accordingly, the documentation
> (http://nginx.org/r/access_log) recommends to specify both root and access_log if you want to use variables in logging:
>
> : during each log write the existence of the request’s root
> : directory is checked, and if it does not exist the log is not
> : created. It is thus a good idea to specify both root and
> : access_log on the same level:
> :
> : server {
> : root /spool/vhost/data/$host;
> : access_log /spool/vhost/logs/$host;
> : ...
>
Thanks for the clarification, IMHO worth having it configurable (check_root=on/off), in my use case,
the variables do not depend on anything a client can control (there will be vars generated by the strftime
module, and static strings from map), so the root check is simply irrelevant.
Will be happy to submit a patch for that later if you agree, though less a priority for me than this patch.

> --
> Maxim Dounin
> http://nginx.org/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel