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".
> - 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.
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.
> === 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?
> 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.
