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
