[squid-dev] [PATCH] Do not die silently when dying early

[Alex Rousskov]

On 05/20/2017 05:04 AM, Amos Jeffries wrote:

> in src/Debug.h:
> 
> * instead of adding yet another wrapper macro, please replace all uses
> of debug_log with DebugStream().

I will not do that because many (most?) debug_log users should not use
DebugStream(). They should use the standard debugs() API. However,
carefully fixing that code is outside this particular change scope.


>  - we already know from earlier macros that these just lead us to much
> more pain and long arguments in future developments.

Compared to HERE and NULL, there are very few debug_log users. The
primary reason I left them alone is that many of them require
non-trivial out-of-scope changes other than dumb renaming. It is
actually a good idea to leave the old macro in place as a marker for
that code.


>  - the important comment by Duane in src/main.cc SquidShutdown() is now
> needing an update to reference the current symbol names.

Since I am refusing to remove debug_log in this patch, the comment does
not need updating. The comment itself should be removed as stale (or
completely rewritten) because (a) Squid provides several services that
must function beyond main() and (b) those services should not have a
"close" API so that we do not need to warn anybody not to use that wrong
API.


> in src/debug.cc:
> 
> * debugOpenLog()
>  - since the bulk of the function is being altered please take the
> opportunity to remove the use of NULL in there.

I am not going to do what I think is wrong and what I continue to
discourage others from doing. The pain reduction rule we all should
follow IMO is "If you do not have to change a line for in-scope reasons,
then do not change that line".


>  - in the else condition after fopen() we are writing explicitly to
> stderr then clearing TheLog. It seems to me that a better thing would be
> to do is write that notice to TheLog before clearing it, to perror() and
> then to TheLog after clearing it.

TheLog is usually closed at the time of that "else" code execution. The
correct solution here is to use debugs(), while making sure that API
works in this (and nearly any other) context and logs to stderr and
syslog as needed. That change is outside this patch scope.

Did I convince you that none of the above requested changes are required?


Thank you,

Alex.