On 6/02/2013 8:14 a.m., Alex Rousskov wrote:
On 02/05/2013 03:36 AM, Amos Jeffries wrote:
http://bugs.squid-cache.org/show_bug.cgi?id=3686
+ if (static_cast<uint64_t>(newMax) > maxSize()) {
+ debugs(47, DBG_PARSE_NOTE(2), "WARNING: object 'max-size' option for " << path <<
" cannot exceed " << maxSize());
+ max_objsize = maxSize();
+ return;
+ }
Please adjust this to say "cannot exceed total cache_dir size " or
similar to clarify where the limit is coming from.
I would also say "ignoring max-size for <path> because it exceeds
cache_dir <size>" instead of the "max-size cannot exceed ..." phrase, to
tell the admin what Squid is doing instead of just telling them what
Squid cannot do. This will make it clear that their max-size option has
no effect.
In summary:
debugs(47, DBG_PARSE_NOTE(2), "WARNING: ignoring max-size for " <<
path << " because it exceeds total cache_dir size " << maxSize());
+ /// the minimum size of singel object which may be stored here
"single" typo
I would rephrase as "smaller objects will not be added and may be purged".
Actually this was completely wrong anyway. minSize() *is* the function
paired with maxSize() and seems to be related to the low watermark somehow.
Dropped the documentation change.
- virtual int64_t maxObjectSize() const { return max_objsize; }
+ /// The maximum size of single object which may be stored here.
+ int64_t maxObjectSize() const;
Same rephrasing suggestion as the above.
Please do not remove "virtual". It is a useful reminder that this is a
part of the Store API.
Why add virtual table entries? this is not a replaceable function any
more. This is an accessor method for the SwapDir base object.
Also, I think it would be appropriate (and useful) to document maxSize()
method in this patch because it looks symmetrical to now-documented
minSize() but has a very different meaning.
N/A now and I'm not sure I understand it well enough to add documentation.
=== modified file 'src/peer_digest.cc'
--- src/peer_digest.cc 2013-01-30 15:39:37 +0000
+++ src/peer_digest.cc 2013-02-03 13:38:13 +0000
@@ -347,6 +347,7 @@
req->header.putStr(HDR_ACCEPT, "text/html");
+ // XXX: this is broken when login=PASS, login=PASSTHRU, login=PROXYPASS, login=NEGOTIATE, and login=*:...
if (p->login)
xstrncpy(req->login, p->login, MAX_LOGIN_SZ);
An unrelated change?
Er. yes. dropped.
StoreEntry *const pe = addEntry(i);
+ printf("pe->swap_status == %d (SWAPOUT_WRITING=%d)\n", pe->swap_status, SWAPOUT_WRITING);
+
CPPUNIT_ASSERT(pe->swap_status == SWAPOUT_WRITING);
CPPUNIT_ASSERT(pe->swap_dirn == 0);
CPPUNIT_ASSERT(pe->swap_filen >= 0);
Perhaps remove the added debugging to reduce "make check" noise? Or make
it conditional on swap_status being not SWAPOUT_WRITING?
+ rep->setHeaders(HTTP_OK, "dummy test object", "x-squid-internal/test", 0,
-1, squid_curtime + 100000);
You may want to add a line to the commit message to note why this change
was necessary (and that test cases now have weak/indirect checks for
max-size code).
All of the above can be done during commit, without another review cycle
IMO.
Thank you,
Alex.
Applie dto trunk as rev.12662.
Amos
