On 07/30/2011 02:36 AM, Amos Jeffries wrote:
On 30/07/11 02:10, Tsantilas Christos wrote:
This is a patch which solves the bug 3118. Because it is not so small I
am posting it here for comments.
Here is the patch description:
Currently we were updating [Icap|Ecap]::TheConfig even when
[icap|ecap]_enable was false, which may lead to service activation for
Icap or Ecap services that should be disabled. The patch removes such
services from service groups before they are activated.
The patch also warns the user when an adaptation group loses some but
not all of its services due to the new group cleanup code.
Regards,
Christos
Can you thread some debugs statements through
removeService(),removeRules() please, somewhere level 4-6.
OK, done
What happens when a group is emptied of services and pruned after its
been linked to or used by AccessRule? or any other config?
The AccessRule removed too.
Amos
author: Measurement Factory
Bug 3118: ecap_enable on forces icap_enable on
We were updating [Icap|Ecap]::TheConfig even when [icap|ecap]_enable was false,
which may lead to service activation for Icap or Ecap services that should be
disabled. The patch removes such services from service groups before they are
activated.
The patch also warns the user when an adaptation group loses some but not all
of its services due to the new group cleanup code.
=== modified file 'src/adaptation/Config.cc'
--- src/adaptation/Config.cc 2011-02-17 19:27:54 +0000
+++ src/adaptation/Config.cc 2011-08-01 10:06:00 +0000
@@ -59,6 +59,63 @@
}
void
+Adaptation::Config::removeService(const String& service)
+{
+ removeRule(service);
+ const Groups& groups = AllGroups();
+ for (unsigned int i = 0; i < groups.size(); ) {
+ const ServiceGroupPointer group = groups[i];
+ const ServiceGroup::Store& services = group->services;
+ typedef ServiceGroup::Store::const_iterator SGSI;
+ for (SGSI it = services.begin(); it != services.end(); ++it) {
+ if (*it == service) {
+ group->removedServices.push_back(service);
+ group->services.prune(service);
+ debugs(93, 5, HERE << "adaptation service " << service <<
+ " removed from group " << group->id);
+ break;
+ }
+ }
+ if (services.empty()) {
+ removeRule(group->id);
+ AllGroups().prune(group);
+ } else {
+ ++i;
+ }
+ }
+}
+
+void
+Adaptation::Config::removeRule(const String& id)
+{
+ typedef AccessRules::const_iterator ARI;
+ const AccessRules& rules = AllRules();
+ for (ARI it = rules.begin(); it != rules.end(); ++it) {
+ AccessRule* rule = *it;
+ if (rule->groupId == id) {
+ debugs(93, 5, HERE << "removing access rules for:" << id);
+ AllRules().prune(rule);
+ delete (rule);
+ break;
+ }
+ }
+}
+
+void
+Adaptation::Config::clear()
+{
+ debugs(93, 3, HERE << "rules: " << AllRules().size() << ", groups: " <<
+ AllGroups().size() << ", services: " << serviceConfigs.size());
+ typedef ServiceConfigs::const_iterator SCI;
+ const ServiceConfigs& configs = serviceConfigs;
+ for (SCI cfg = configs.begin(); cfg != configs.end(); ++cfg)
+ removeService((*cfg)->key);
+ serviceConfigs.clean();
+ debugs(93, 3, HERE << "rules: " << AllRules().size() << ", groups: " <<
+ AllGroups().size() << ", services: " << serviceConfigs.size());
+}
+
+void
Adaptation::Config::parseService()
{
ServiceConfigPointer cfg = newServiceConfig();
@@ -94,9 +151,14 @@
}
}
-void
+bool
Adaptation::Config::finalize()
{
+ if (!onoff) {
+ clear();
+ return false;
+ }
+
// create service reps from service configs
int created = 0;
@@ -120,6 +182,7 @@
// services remember their configs; we do not have to
serviceConfigs.clean();
+ return true;
}
// poor man for_each
=== modified file 'src/adaptation/Config.h'
--- src/adaptation/Config.h 2011-03-08 23:56:22 +0000
+++ src/adaptation/Config.h 2011-07-29 13:32:42 +0000
@@ -54,12 +54,27 @@
void dumpService(StoreEntry *, const char *) const;
ServicePointer findService(const String&);
- virtual void finalize();
+ /**
+ * Creates and starts the adaptation services. In the case the adaptation
+ * mechanism is disabled then removes any reference to the services from
+ * access rules and service groups, and returns false.
+ * \return true if the services are ready and running, false otherwise
+ */
+ virtual bool finalize();
protected:
+ /// Removes any reference to the services from configuration
+ virtual void clear();
+
/// creates service configuration object that will parse and keep cfg info
virtual ServiceConfig *newServiceConfig() const;
+ /// Removes the given service from all service groups.
+ void removeService(const String& service);
+
+ /// Removes access rules of the given service or group
+ void removeRule(const String& id);
+
private:
Config(const Config &); // unsupported
Config &operator =(const Config &); // unsupported
=== modified file 'src/adaptation/ServiceGroups.cc'
--- src/adaptation/ServiceGroups.cc 2011-03-30 17:43:55 +0000
+++ src/adaptation/ServiceGroups.cc 2011-07-29 13:06:49 +0000
@@ -39,6 +39,16 @@
// 2) warn if all-same services have different bypass status
// 3) warn if there are seemingly identical services in the group
// TODO: optimize by remembering ServicePointers rather than IDs
+ if (!removedServices.empty()) {
+ String s;
+ for (Store::iterator it = removedServices.begin(); it != removedServices.end(); ++it) {
+ s.append(*it);
+ s.append(',');
+ }
+ s.cut(s.size() - 1);
+ debugs(93, DBG_IMPORTANT, "Adaptation group '" << id << "' contains disabled member(s) after reconfiguration: " << s);
+ removedServices.clean();
+ }
String baselineKey;
bool baselineBypass = false;
=== modified file 'src/adaptation/ServiceGroups.h'
--- src/adaptation/ServiceGroups.h 2011-03-30 17:43:55 +0000
+++ src/adaptation/ServiceGroups.h 2011-07-29 13:34:32 +0000
@@ -56,6 +56,7 @@
String kind;
Id id;
Store services;
+ Store removedServices;///< the disabled services in the case ecap or icap is disabled
Method method; /// based on the first added service
VectPoint point; /// based on the first added service
=== modified file 'src/adaptation/ecap/Config.cc'
--- src/adaptation/ecap/Config.cc 2011-03-08 23:56:22 +0000
+++ src/adaptation/ecap/Config.cc 2011-07-29 13:06:49 +0000
@@ -18,12 +18,14 @@
{
}
-void
+bool
Adaptation::Ecap::Config::finalize()
{
- Adaptation::Config::finalize();
+ if (!Adaptation::Config::finalize())
+ return false;
Host::Register();
CheckUnusedAdapterServices(AllServices());
+ return true;
}
Adaptation::ServiceConfig *
=== modified file 'src/adaptation/ecap/Config.h'
--- src/adaptation/ecap/Config.h 2011-03-10 01:12:44 +0000
+++ src/adaptation/ecap/Config.h 2011-07-29 13:06:49 +0000
@@ -38,7 +38,7 @@
Config();
~Config();
- virtual void finalize();
+ virtual bool finalize();
protected:
virtual Adaptation::ServiceConfig *newServiceConfig() const;
