Re: [MERGE] SourceLayout: logs take 0

Alex Rousskov wrote:
> On 03/19/2009 06:48 PM, Amos Jeffries wrote:
>> Shuffle the logs output code into liblogs.la
>>
>> Shuffles the log output files into src/logs.
>
> Are there any build targets that use COMMON_LIBS but do not use
> liblogs.la? If not, you can move loblogs into COMMON_LIBS.

AFAIK, liblogs should only one needed by Squid binary.

I'll look into how many places I left useless links...

>
>> Also C++ converts logfile* and customlog* functions into methods of
>> classes LogFile and CustomLog.
>
>> LogFile objects have a 1:1 relationship with disk files
>
> Except for syslog-based logging?

Ah yes. In which case LogFile:syslog has N:1 relationship ;)

>
> Do you think it would be better to handle syslog in a child class of
> LogFile or Log? There seems to be a lot of "exceptions" and testing for
> syslog in the current general LogFile code...

For now I'm doing minimal conversion of the codepaths themselves. The
LogDaemon work for 3.2 does breaking out into child classes and various
cleaning up of the code here.

>
>> and control all
>> actions associated with that file. Log files are created by passing the
>> details to a LogFile constructor and when constructor has completed the
>> LogFile object will be immediately writable as output logs. Files are
>> closed by deleting the LogFile handler object.
>
> The LogFile class is missing a brief description of its primary purpose,
> I think. All newly added classes should have that right above the class
> declaration.

Mea culpa.

>
> The LogFile class is missing a copy ctor and an assignment operator. If
> you do not need them, declare but do not define them.

Done.

>
> Please add preliminary member initialization to LogFile constructor.
> There may be no bugs now, but it is difficult to say whether everything
> is initialized in that long ctor with many ifs and #ifdefs...
>
> Non-static LogFile method names should start with a lower-case letter,
> right? Any reason to keep them out-of-style with other new classes?
>

Oops. done.

>
>> CustomLog objects are generated from configuration info and describe the
>> log file and its display format.
>
> The CustomLog class is missing a brief description of its primary
> purpose, I think. All newly added classes should have that. It would be
> nice if that description mentions the relationship between CustomLog and
> the default log. That part confuses developers like me. If CustomLog
> handles all types of logs, including the default one, then why is it
> called custom?

I'm not too clear on that one myself even after reading the code a lot.

I just converted the functions named 'customlogXX' to its members. I
think its named that way because its the logformat handler and as such
allows 'custom' log formats to be defined. We actually use it for
several of the logs now whch have to hard-coded format.

>
> CustomLog class is missing a copy ctor and an assignment operator. If
> you do not need them, declare but do not define them.
>
> Do we need class LogConfig? One would expect to find log configuration
> there, but the class does not have any data members, holds just one
> [static] method. Yet, we have a global object of LogConfig type.
> Something is odd here. This should be a namespace or just a global
> function, I guess.

Theres bits still to shuffle in there. Thanks for the reminder.

>
>> NP: Simple open+write testing has been done to verify cache.log output.
>> However bugs in HEAD prevent proper testing at this time.
>
> Does "make distcheck" still work if translations are disabled?
>

Um, running now after adjustments...

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE6 or 3.0.STABLE13
   Current Beta Squid 3.1.0.6