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

Alex Rousskov rousskov at measurement-factory.com
Fri Jun 2 00:43:34 UTC 2017


On 05/20/2017 10:31 AM, Alex Rousskov wrote:
> 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?


Amos, are you OK with this patch going in "as is" for the reasons given
above? If not, please see if those reasons have affected any of your
change requests so that we can make progress.


Thank you,

Alex.


More information about the squid-dev mailing list