Re: [PATCH] Ignore and warn about attempts to reconfigure static Rock store options.
On 09/19/2011 08:10 PM, Amos Jeffries wrote: > On Tue, 20 Sep 2011 02:40:13 +0400, Dmitry Kurochkin wrote: >> Attached patch does some polishing for Rock store cache_dir >> reconfiguration. >> >> >> Some Rock store options cannot be changed dynamically: path, size, and >> max-size. Before the change, there were no checks during reconfigure >> to prevent changing these options. This may lead to Rock cache >> corruption and other bugs. The patch adds necessary checks to Rock >> store code. If user tries to change an option that cannot be updated >> dynamically, a warning is reported and the value is left unchanged. > Of itself the patch looks okay. > > BUT... IMO _path_ should never be re-configurable. Good point! I wonder whether admins expect cache_dir lines to be tied to paths (e.g., changing cache_dir order changes nothing) or to position (i.e., changing path can be supported). We should document what we actually support (but that documentation change, if needed, is outside this patch scope). > If rock store can't handle multiple cache_dir of type rock with unique > paths there is something deeper wrong. It can. Nothing special here. > AFAIK path should be the unique key to identify different cache_dir. > Changing the path means a completely different dir is now being > referenced. If not already loaded the dir needs to be loaded as if on > startup. If any existing are no longer referenced the old cache_dir > details needs discarding. I tend to agree: Path-matching is probably better than position-matching. Dmitry, could you please check whether current code maps cache_dir lines to SwapDir objects using cache_dir position in squid.conf or using cache_dir paths? If it is the latter, you can remove the path-related warning from the patch, right? Thank you, Alex.
Re: question about the key generation when Squid start to store object in cache?
On Tue, 20 Sep 2011 10:47:31 +0800, Raymond Wang wrote: Hello , I'd like to know the mechanism about the key generation, and how could I config the key generation process, and it is better if I What is it you are trying to do? there is nothing key-related that can safely be left to Squid admin choices. There are some improvements that can be made in the squid Vary: and Etag: handling. Perhapse you are interested in discussing and improving those? get the related code so that I can view the mechanism in action. Thanks a lot in advance! Store key is an MD5 hash of the client request details for which the stored object is a possible response. The code is at: http://bazaar.launchpad.net/~squid/squid/3-trunk/view/head:/src/store_key_md5.cc The code which uses it is in: http://bazaar.launchpad.net/~squid/squid/3-trunk/view/head:/src/store.cc StoreEntry::setPublicKey() does most of the work generating new index entries. Amos
question about the key generation when Squid start to store object in cache?
Hello , I'd like to know the mechanism about the key generation, and how could I config the key generation process, and it is better if I get the related code so that I can view the mechanism in action. Thanks a lot in advance! -- Best Regards rmn190
Re: [PATCH] Ignore and warn about attempts to reconfigure static Rock store options.
On Tue, 20 Sep 2011 02:40:13 +0400, Dmitry Kurochkin wrote: Hi. Attached patch does some polishing for Rock store cache_dir reconfiguration. Some Rock store options cannot be changed dynamically: path, size, and max-size. Before the change, there were no checks during reconfigure to prevent changing these options. This may lead to Rock cache corruption and other bugs. The patch adds necessary checks to Rock store code. If user tries to change an option that cannot be updated dynamically, a warning is reported and the value is left unchanged. Regards, Dmitry Of itself the patch looks okay. BUT... IMO _path_ should never be re-configurable. That may impact the patch implementation. If rock store can't handle multiple cache_dir of type rock with unique paths there is something deeper wrong. AFAIK path should be the unique key to identify different cache_dir. Changing the path means a completely different dir is now being referenced. If not already loaded the dir needs to be loaded as if on startup. If any existing are no longer referenced the old cache_dir details needs discarding. Amos
Re: [PATCH] Fix cache_dir type check during reconfiguration.
On Tue, 20 Sep 2011 13:44:24 +1200, Amos Jeffries wrote: On Tue, 20 Sep 2011 02:38:08 +0400, Dmitry Kurochkin wrote: Hello. This is a small and simple fix for cache_dir reconfiguration. SwapDir::type() returns C strings which should be compared with strcmp(3) instead of checking pointers for equality. Regards, Dmitry eek! +1 please commit. Amos Actually. Please check if this fixes: http://bugs.squid-cache.org/show_bug.cgi?id=3054 and reference the bug the usual way if it does. Amos
Re: [PATCH] Fix cache_dir type check during reconfiguration.
On Tue, 20 Sep 2011 02:38:08 +0400, Dmitry Kurochkin wrote: Hello. This is a small and simple fix for cache_dir reconfiguration. SwapDir::type() returns C strings which should be compared with strcmp(3) instead of checking pointers for equality. Regards, Dmitry eek! +1 please commit. Amos
Re: [PATCH] Fix session helper "crashing too rapidly"
On Mon, 19 Sep 2011 23:45:58 +0100, Andrew Beverley wrote:
On Mon, 2011-09-19 at 10:49 +1200, Amos Jeffries wrote:
The session helper in Squid-3 is concurrent.
Ah, okay.
The user_key is the opaque
channel-ID. (Probably should be renamed to match the protocol
documentation).
http://wiki.squid-cache.org/Features/AddonHelpers#Access_Control_.28ACL.29
I've renamed to channel_id
The correct way to fix this is to detect the segfault case add a
stderr
ERROR: message that the helper is concurrent and requires a config
update.
I've added a fatal error message to STDERR, but I can't get it to
output
anywhere (see below).
stderr should appear in cache.log whenever sent.
Hmmm, it doesn't seem to be.
Most of the lines so
far appear to be debug messages
Does that include the failure to open a database? This is written to
STDERR as per any other message, but it does not make it anywhere.
, which depend on the -d option to
display.
Do you mean the -d option to the Squid binary? If so, this doesn't
seem
to make any difference; it just prints all the log messages to the
display as well as the log file.
-d parameter of the helper binary. stderr is piped to cache.log at
level-0. Thus the need for a separate switch to enable lots of debug
from the helper.
I've tried increasing the log level to 9, but to no avail.
Alsothe .8 manual needs to mention the concurrency rather than
just
implying it in the example config.
Done. Also, the following page should be updated:
http://wiki.squid-cache.org/ConfigExamples/Portal/Splash
I'm happy to do it myself, if you can give me wiki edit rights?
Sure. Just need to know what username you login with.
The helper version should get a bump
to 1.1 as well.
Done in the manual. Is it specified anywhere else as well?
I've also added:
- A new option: "-h". This causes a "hard" timeout, regardless of
user
activity. This is because, for many uses of a splash page (adverts
etc)
one would want the message to displayed every n hours, regardless of
user activity (certainly that is my requirement).
-h is reserved for display of "help" / usage() texts.
-T would probably be better for an absolute timeout.
- A db->sync command. I have found that with more than one child
started, that they do not necessarily share data because each child's
data has not been flushed to disk. This fixes that. However, I am not
sure whether it has other implications, such as much greater disk
overhead. Do you think it should be a configurable option?
IMO that is just something that should happen. So not configurable
unless we have to disable it for someone.
Is there anything else that needs updating? RELEASENOTES.html?
Not at this point. I'll get the Changelog entry during release
processing.
Please find attached updated patch for comment.
Okay. The audit (ominous drums):
ext_session_acl.8:
* " (unless -h is specified).". Please make a separate sentence.
Something like "Also supports fixed length timeouts for regular splash
page displays."
(if you can think of any other useful scenario than regular splash
pages that can be mentioned too.)
* rather than calling the new mode "Hard timeout". It's a "Periodic"
or "Fixed length" timeout ... something a bit more descriptive like
that.
** Needs to explain how it interacts with -a mode. ie I would expect
LOGOUT or timeout to terminate the session. Explicit LOGIN required for
a new one to start.
ext_session_acl.cc: (modulo the -h ==> -T change requested)
* in getopt() format syntax ':' indicates a value is received for the
option. What you coded is "t:b:a?h?".
* I think you should make -t and -T both optional arguments which are
interchangeable. ("t:T:b:a?")
case 'T':
hard-timeout=1;
case 't':
... process timeout value
break;
Other than that okay.
Amos
Re: [PATCH] Fix session helper "crashing too rapidly"
On Mon, 2011-09-19 at 10:49 +1200, Amos Jeffries wrote:
> The session helper in Squid-3 is concurrent.
Ah, okay.
> The user_key is the opaque
> channel-ID. (Probably should be renamed to match the protocol
> documentation).
> http://wiki.squid-cache.org/Features/AddonHelpers#Access_Control_.28ACL.29
I've renamed to channel_id
> The correct way to fix this is to detect the segfault case add a stderr
> ERROR: message that the helper is concurrent and requires a config
> update.
I've added a fatal error message to STDERR, but I can't get it to output
anywhere (see below).
> stderr should appear in cache.log whenever sent.
Hmmm, it doesn't seem to be.
> Most of the lines so
> far appear to be debug messages
Does that include the failure to open a database? This is written to
STDERR as per any other message, but it does not make it anywhere.
> , which depend on the -d option to
> display.
Do you mean the -d option to the Squid binary? If so, this doesn't seem
to make any difference; it just prints all the log messages to the
display as well as the log file.
I've tried increasing the log level to 9, but to no avail.
> Alsothe .8 manual needs to mention the concurrency rather than just
> implying it in the example config.
Done. Also, the following page should be updated:
http://wiki.squid-cache.org/ConfigExamples/Portal/Splash
I'm happy to do it myself, if you can give me wiki edit rights?
> The helper version should get a bump
> to 1.1 as well.
Done in the manual. Is it specified anywhere else as well?
I've also added:
- A new option: "-h". This causes a "hard" timeout, regardless of user
activity. This is because, for many uses of a splash page (adverts etc)
one would want the message to displayed every n hours, regardless of
user activity (certainly that is my requirement).
- A db->sync command. I have found that with more than one child
started, that they do not necessarily share data because each child's
data has not been flushed to disk. This fixes that. However, I am not
sure whether it has other implications, such as much greater disk
overhead. Do you think it should be a configurable option?
Is there anything else that needs updating? RELEASENOTES.html?
Please find attached updated patch for comment.
Thanks,
Andy
=== modified file 'helpers/external_acl/session/ext_session_acl.8'
--- helpers/external_acl/session/ext_session_acl.8 2010-09-16 13:07:02 +
+++ helpers/external_acl/session/ext_session_acl.8 2011-09-19 22:42:10 +
@@ -1,11 +1,11 @@
-.if !'po4a'hide' .TH ext_session_acl 8 "19 March 2006"
+.if !'po4a'hide' .TH ext_session_acl 8 "19 September 2011"
.
.SH NAME
.if !'po4a'hide' .B ext_session_acl
.if !'po4a'hide' \-
Squid session tracking external acl helper.
.PP
-Version 1.0
+Version 1.1
.
.SH SYNOPSIS
.if !'po4a'hide' .B ext_session_acl
@@ -19,7 +19,7 @@
.B ext_session_acl
maintains a concept of sessions by monitoring requests
and timing out sessions if no requests have been seen for the idle timeout
-timer.
+timer (unless -h is specified).
.PP
Intended use is for displaying "terms of use" pages, ad popups etc.
.
@@ -34,7 +34,7 @@
.B Path
to persistent database. If not specified the session details
will be kept in memory only and all sessions will reset each time
-Squid restarts it's helpers (Squid restart or rotation of logs).
+Squid restarts its helpers (Squid restart or rotation of logs).
.
.if !'po4a'hide' .TP
.if !'po4a'hide' .B \-a
@@ -43,12 +43,22 @@
.B LOGIN
, or terminated by the argument
.B LOGOUT
-.PP
Without this flag the helper automatically starts the session after
the first request.
.
+.if !'po4a'hide' .TP
+.if !'po4a'hide' .B \-h
+Hard timeout mode. In this mode sessions only last for
+.B timeout
+seconds, regardless of the user's activity.
.SH CONFIGURATION
.PP
+The
+.B ext_session_acl
+helper is a concurrent helper; therefore, the concurrency= option
+.B must
+be specified in the configuration.
+.PP
Configuration example using the default automatic mode
.if !'po4a'hide' .RS
.if !'po4a'hide' .B external_acl_type session ttl=300 negative_ttl=0 children=1 concurrency=200 %LOGIN /usr/local/squid/libexec/ext_session_acl
=== modified file 'helpers/external_acl/session/ext_session_acl.cc'
--- helpers/external_acl/session/ext_session_acl.cc 2010-07-29 14:23:35 +
+++ helpers/external_acl/session/ext_session_acl.cc 2011-09-19 22:42:03 +
@@ -52,6 +52,7 @@
#endif
static int session_ttl = 3600;
+static int hard_timeout = 0;
char *db_path = NULL;
const char *program_name;
@@ -101,6 +102,7 @@
data.data = &now;
data.size = sizeof(now);
db->put(db, &key, &data, 0);
+db->sync(db, 0);
}
static void session_logout(const char *details, size_t len)
@@ -113,10 +115,11 @@
static void usage(void)
{
-fprintf(stderr, "Usage: %s [-t session_timeout] [-b dbpath] [-a]\n", program_name);
+fprintf(stderr, "Usage: %s [-t session_timeout] [-b dbpath] [-a] [-h]\n", progra
[PATCH] Ignore and warn about attempts to reconfigure static Rock store options.
Hi.
Attached patch does some polishing for Rock store cache_dir
reconfiguration.
Some Rock store options cannot be changed dynamically: path, size, and
max-size. Before the change, there were no checks during reconfigure
to prevent changing these options. This may lead to Rock cache
corruption and other bugs. The patch adds necessary checks to Rock
store code. If user tries to change an option that cannot be updated
dynamically, a warning is reported and the value is left unchanged.
Regards,
Dmitry
Ignore and warn about attempts to reconfigure static Rock store options.
Some Rock store options cannot be changed dynamically: path, size, and
max-size. Before the change, there were no checks during reconfigure
to prevent changing these options. This may lead to Rock cache
corruption and other bugs. The patch adds necessary checks to Rock
store code. If user tries to change an option that cannot be updated
dynamically, a warning is reported and the value is left unchanged.
=== modified file 'src/SwapDir.cc'
--- src/SwapDir.cc 2011-05-12 03:58:16 +
+++ src/SwapDir.cc 2011-09-19 22:15:05 +
@@ -293,42 +293,51 @@ SwapDir::optionReadOnlyDump(StoreEntry *
if (flags.read_only)
storeAppendPrintf(e, " no-store");
}
bool
SwapDir::optionObjectSizeParse(char const *option, const char *value, int isaReconfig)
{
int64_t *val;
if (strcmp(option, "max-size") == 0) {
val = &max_objsize;
} else if (strcmp(option, "min-size") == 0) {
val = &min_objsize;
} else
return false;
if (!value)
self_destruct();
int64_t size = strtoll(value, NULL, 10);
-if (isaReconfig && *val != size)
-debugs(3, 1, "Cache dir '" << path << "' object " << option << " now " << size);
+if (isaReconfig && *val != size) {
+if (allowOptionReconfigure(option)) {
+debugs(3, DBG_IMPORTANT, "cache_dir '" << path << "' object " <<
+ option << " now " << size << " Bytes");
+} else {
+debugs(3, DBG_IMPORTANT, "WARNING: cache_dir '" << path << "' "
+ "object " << option << " cannot be changed dynamically, " <<
+ "value left unchanged (" << *val << " Bytes)");
+return true;
+}
+}
*val = size;
return true;
}
void
SwapDir::optionObjectSizeDump(StoreEntry * e) const
{
if (min_objsize != 0)
storeAppendPrintf(e, " min-size=%"PRId64, min_objsize);
if (max_objsize != -1)
storeAppendPrintf(e, " max-size=%"PRId64, max_objsize);
}
// some SwapDirs may maintain their indexes and be able to lookup an entry key
StoreEntry *
SwapDir::get(const cache_key *key)
{
=== modified file 'src/SwapDir.h'
--- src/SwapDir.h 2011-05-12 03:58:16 +
+++ src/SwapDir.h 2011-09-19 17:57:28 +
@@ -145,40 +145,41 @@ public:
virtual uint64_t minSize() const;
virtual int64_t maxObjectSize() const { return max_objsize; }
virtual void stat (StoreEntry &anEntry) const;
virtual StoreSearch *search(String const url, HttpRequest *) = 0;
/* migrated from store_dir.cc */
bool objectSizeIsAcceptable(int64_t objsize) const;
/// called when the entry is about to forget its association with cache_dir
virtual void disconnect(StoreEntry &) {}
/// called when entry swap out is complete
virtual void swappedOut(const StoreEntry &e) = 0;
protected:
void parseOptions(int reconfiguring);
void dumpOptions(StoreEntry * e) const;
virtual ConfigOption *getOptionTree() const;
+virtual bool allowOptionReconfigure(const char *const) const { return true; }
int64_t sizeInBlocks(const int64_t size) const { return (size + fs.blksize - 1) / fs.blksize; }
private:
bool optionReadOnlyParse(char const *option, const char *value, int reconfiguring);
void optionReadOnlyDump(StoreEntry * e) const;
bool optionObjectSizeParse(char const *option, const char *value, int reconfiguring);
void optionObjectSizeDump(StoreEntry * e) const;
char const *theType;
protected:
uint64_t max_size;///< maximum allocatable size of the storage area
public:
char *path;
int index; /* This entry's index into the swapDirs array */
int64_t min_objsize;
int64_t max_objsize;
RemovalPolicy *repl;
int removals;
=== modified file 'src/fs/rock/RockSwapDir.cc'
--- src/fs/rock/RockSwapDir.cc 2011-09-14 19:59:30 +
+++ src/fs/rock/RockSwapDir.cc 2011-09-19 22:14:38 +
@@ -237,80 +237,97 @@ Rock::SwapDir::init()
}
bool
Rock::SwapDir::needsDiskStrand() const
{
return true;
}
void
Rock::SwapDir::parse(int anIndex, char *aPath)
{
index = anIndex;
path = xstrdup(aPath);
// cache store is located at path/db
String fname(path);
fname.append("/rock");
filePath = xstrdup(fname.termedBuf());
-parseSize();
+parseSize(false);
parseOptions(0);
[PATCH] Fix cache_dir type check during reconfiguration.
Hello.
This is a small and simple fix for cache_dir reconfiguration.
SwapDir::type() returns C strings which should be compared with
strcmp(3) instead of checking pointers for equality.
Regards,
Dmitry
Fix cache_dir type check during reconfiguration.
SwapDir::type() returns C strings which should be compared with
strcmp(3) instead of checking pointers for equality.
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2011-09-10 04:22:16 +
+++ src/cache_cf.cc 2011-09-19 22:11:00 +
@@ -1936,41 +1936,41 @@ parse_cachedir(SquidConfig::_cacheSwap *
fs = find_fstype(type_str);
if (fs < 0)
self_destruct();
/* reconfigure existing dir */
for (i = 0; i < swap->n_configured; i++) {
assert (swap->swapDirs[i].getRaw());
if ((strcasecmp(path_str, dynamic_cast(swap->swapDirs[i].getRaw())->path)) == 0) {
/* this is specific to on-fs Stores. The right
* way to handle this is probably to have a mapping
* from paths to stores, and have on-fs stores
* register with that, and lookip in that in their
* own setup logic. RBC 20041225. TODO.
*/
sd = dynamic_cast(swap->swapDirs[i].getRaw());
-if (sd->type() != StoreFileSystem::FileSystems().items[fs]->type()) {
+if (strcmp(sd->type(), StoreFileSystem::FileSystems().items[fs]->type()) != 0) {
debugs(3, 0, "ERROR: Can't change type of existing cache_dir " <<
sd->type() << " " << sd->path << " to " << type_str << ". Restart required");
return;
}
sd->reconfigure (i, path_str);
update_maxobjsize();
return;
}
}
/* new cache_dir */
if (swap->n_configured > 63) {
/* 7 bits, signed */
debugs(3, DBG_CRITICAL, "WARNING: There is a fixed maximum of 63 cache_dir entries Squid can handle.");
debugs(3, DBG_CRITICAL, "WARNING: '" << path_str << "' is one to many.");
self_destruct();
return;
Re: [PATCH] Bug 3299 - detatch dnsserver from Squid libraries
On Mon, 19 Sep 2011 12:10:35 -0600, Alex Rousskov wrote: On 09/18/2011 06:21 AM, Amos Jeffries wrote: This is step one of cleaning up the DNS system according to SourceLayout. Breaking the dependencies dnsserver has to Ip::Address and thus all the libmisc* libraries etc which get pulled in by chain reaction. I am surprised you want Squid code to avoid using IP::Address and other useful "OS primitive" wrappers. I do not see a problem with using useful basic libraries and duplicating some of their code seems like a step backwards to me, but I do not have any strong opinions regarding dnsserver executable in particular. Alex. Yes I am a bit disappointed at having to do this. When we get the boundaries between debugs(), time, new() and DNS ops set cleanly this can hopefully be reversed. Amos
Re: [PATCH] Bug 3299 - detatch dnsserver from Squid libraries
On 09/18/2011 06:21 AM, Amos Jeffries wrote: > This is step one of cleaning up the DNS system according to SourceLayout. > Breaking the dependencies dnsserver has to Ip::Address and thus all the > libmisc* libraries etc which get pulled in by chain reaction. I am surprised you want Squid code to avoid using IP::Address and other useful "OS primitive" wrappers. I do not see a problem with using useful basic libraries and duplicating some of their code seems like a step backwards to me, but I do not have any strong opinions regarding dnsserver executable in particular. Alex.
