Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2495 fix.

Tsantilas Christos wrote:
> Alex Rousskov wrote:
>> On 07/09/2009 12:06 AM, Amos Jeffries wrote:
>>> Tsantilas Christos wrote:

>>> ** Please make fqdncache_entry / ipcache_entry public classes instead
>>> of functional structs. Some gcc complain and doxygen won't document
>>> their functions properly.
>>
>> Sorry, I am not sure what you mean. What is a "functional struct"? Do
>> you want us to replace "struct fqdncache_entry" with "class
>> fqdncache_entry" and add "public:"?
>>
>> No objections, of course, just wanted to minimize merging headaches by
>> keeping out-of-scope changes to the minimum...
>>
> I did not touch it. The struct used in many places inside squid3
>

Please do change.

The rest of the code fails to prefix the names with "struct" the typedef
made that possible.

Using class and public: is the C++ upgrade equivalent to the typedef and
allows functions/methods inside the struct where I suspect the typedef
did not.

If there are code references to "struct fqdncache_entry" or "struct
ipcache_entry", they are well within scope of the change removing
relevant typedefs.
FWIW: I find none in the existing code. Which makes the change safe.

>>
>>>> I also have some questions to developers:
>>>> - In squid.conf documentation I prepend each
>>>> HTTP format code with [http::]
>>>> Is it OK?
>>> Most of them yes.
>>>
>>> There are a few where http:: does not make sense at all:
>>> ts Seconds since epoch
>>> tu subsecond time (milliseconds)
>>> tl Local time. Optional strftime format argument
>>> tg GMT time. Optional strftime format argument
>>
>> Definitely.
>>
>>> dt Total time spent making DNS lookups (milliseconds)
>>> ui User name from ident
>>> us User name from SSL
>>
>> Yeah, at least for now.
>>
>>> These are also borderline:
>>> >a Client source IP address
>>> >A Client FQDN
>>> >p Client source port
>>> la Local IP address (http_port)
>>> lp Local port number (http_port)
>>> <A Server IP address or peer name
>>
>> Agreed.
>>
>>> I'm fine with ignoring http:: for now if people do enter it for these,
>>> so no coding change. But I don't think we should document them that way.
>>
>> Agreed.
>>
>>> I'd shift the clearly HTTP ones out to a section like you have ICAP.
>>> Leaving the rest prefix-ambiguous until something is done properly.
>>
>> Or at least remove the http:: prefix in the documentation from those
>> mentioned above.
>>
>
> I remove the http:: prefix for the format codes mentioned above.
> But maybe the use of "http::" prefix needs more discussion...
>

Sure.

>>
>>> adaptation::sum_trs / adaptation::tt
>>
>> These are very different! Tt is a single value while adapt::sum_trs is a
>> list (of individual tr's). Adapt::sum_trs is also symmetric with
>> adapt::all_trs (also a list, but possibly with more entries).
>>
>>> Yes they are not ideal, but they are there and what people are used to
>>> for now. We can minimize confusion a bit.
>>
>> OK.
> done for total_time and size
>>
>>
>>> I think at the point you do:
>>> + case LFT_HTTP_SENT_STATUS_CODE_OLD_30:
>>> + debugs(46, 0, "WARNING: the \"Hs\" formating code is deprecated
>>> use the \">Hs\" instead");
>>> + break;
>>>
>>> you could also set
>>> "lt->type = LFT_HTTP_SENT_STATUS_CODE;" to reduce the need for that
>>> OLD_30 type being used outside the parse. Particularly during the config
>>> dump later it is useful to display what its supposed to be.
>>
>> Sounds good.
>
> My compiler (because it is very clever @#%$##$%#!) does not allow me to
> remove the LFT_HTTP_SENT_STATUS_CODE_OLD_30 from switch statements.

Is that because of a lack of default: ?

Never mind then. Just add a note saying what compiler complains if its
removed so I don't get just as smart and do it :)

>>
>>> I spy a goto that can die...
>>> case CLF_NONE:
>>> goto last;
>>> @@ -1458,7 +1863,15 @@
>>> last:
>>> (void)0; /* NULL statement for label */
>>> +}
>>>
>>> ==> equates to "return;". Please check for others in that same
>>> function.
>
> I prefer to not touch it at this time. We can change it in trunk with a
> separate patch.

Okay. Fair enough.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16
   Current Beta Squid 3.1.0.9