Re: [PATCH] [RFC] custom error pages

2010-11-25 Thread Henrik Nordström
tor 2010-11-25 klockan 15:51 -0700 skrev Alex Rousskov:

> Opening files for each runtime error is pretty bad from performance 
> point of view, although we can hope that errors do not happen often 
> enough to cause problems in most setups. Preloading is a better 
> approach, IMO, but is outside this patch scope.

We could cache them as needed.

Regards
Henrik



Re: [PATCH] [RFC] custom error pages

2010-11-25 Thread Alex Rousskov

On 11/21/2010 05:21 PM, Amos Jeffries wrote:

On Sun, 21 Nov 2010 14:50:20 +0100, Henrik Nordström
  wrote:

lör 2010-11-20 klockan 18:58 +1300 skrev Amos Jeffries:


To simplify the error page loading a symlink is added from the

templated

/usr/share/squid/errors/local to that /etc/squid/errors.d folder and
checked just before loading the errors/templates/ backup.


Why? I find having this symlink confusing, and yet another potential
source of configuration errors.


Okay. As Robert pointed out the symlink is not going to work everywhere.
If the idea is agreed I'll go back to an older idea of a separate full
lookup path and some further performance optimisations.


As to why. We currently have admin who still:
  a) want to replace the entire template set with something else
  b) want to replace just some of the template set with something else
  c) want to add local deny_info ERR_* pages with alternative names

The error_directory + auto-negotiate currently caters for (a) and (c) but
not (b).


For (b), the admin who wants to replace some errors, can already do that 
from the Squid startup script:


   cp -r standard_errors/* error_directory/
   cp -r my_errors/* error_directory/
   start squid

Also, the error_directory option in squid.conf allows more flexibility 
for multi-instance and SMP Squids than the proposed hard-coded directory 
name. I think there are cases where different Squid instances can 
benefit from different error pages.


Are you sure replacing error_directory with a hard-coded location is a 
step forward?




This replaces error_directory with a fixed location which can be used to
override auto-negotiate for cases (a) and (b). And also as a known location
for case (c).

My earlier idea was to let them add to errors/templates/ but some distros
need to remove or replace that folder when upgrading the langpack content.

Placing this under /etc/squid/errors.d keeps everything which is manually
configurable in one area. So the admin no longer even really needs to worry
about /usr/shared/squid/errors/ or what is in there.


A custom script like the sketch shown above would accomplish the same, 
right?




The patch here is missing a few possible performance optimizations.
  The first I intend to get in with a re-submission if this idea is
accepted. That is to add a flag to the template data loaded on
initialization to indicate whether its used unconditionally (override) or
auto-negotiate is to happen first (presently only loaded as the final
templates/* backup). This removes 1-2 cycles of file lookup on each error
request.
  Second was suggested by Adrian way back. To load the entire template set
into a memory tree and search that. This is a bit more work and requires
some performance/cost benefit analysis I don't have time for.
  Third is a smaller alternative to the second, just to cache the languages
which are available. Thus reducing some useless file probes. We have most
of the major web languages and their variants, but there are a few which
still hit two file loads.


Opening files for each runtime error is pretty bad from performance 
point of view, although we can hope that errors do not happen often 
enough to cause problems in most setups. Preloading is a better 
approach, IMO, but is outside this patch scope.


AFAICT, your patch will add to that runtime file opening overhead, even 
if the user does not have any custom pages, because Squid will need to 
check whether there is a custom file in the new directory. Not a big 
deal if we find file opening overhead acceptable in general, but perhaps 
it should be documented in the commit message.


Thank you,

Alex.


Re: [PATCH] [RFC] custom error pages

2010-11-25 Thread Alex Rousskov

On 11/19/2010 10:58 PM, Amos Jeffries wrote:

It has become clear that even with the new langpack setups we need an
area dedicated to local custom ERR_* pages which is integrated with but
fully separate from the bundled translations.

This patch adds a configuration directory /etc/squid/errors.d/ for such
templates to be placed.

To simplify the error page loading a symlink is added from the templated
/usr/share/squid/errors/local to that /etc/squid/errors.d folder and
checked just before loading the errors/templates/ backup.

Also, other ideas on what to call the symlink are welcome. I'm kind of
partial to "local", but if anyone has a better idea we can go with that.

Due to this implementation prioritizing these templates it obsoletes
errors_directory. Docs there are updated to deprecate it for future full
removal.


"local" sounds good to me, but can you explain why do we need both 
errors.d and local? Can users install their custom errors into local 
directly?


Thank you,

Alex.


Re: [PATCH] [RFC] custom error pages

2010-11-21 Thread Amos Jeffries
On Sun, 21 Nov 2010 14:50:20 +0100, Henrik Nordström
 wrote:
> lör 2010-11-20 klockan 18:58 +1300 skrev Amos Jeffries:
> 
>> To simplify the error page loading a symlink is added from the
templated 
>> /usr/share/squid/errors/local to that /etc/squid/errors.d folder and 
>> checked just before loading the errors/templates/ backup.
> 
> Why? I find having this symlink confusing, and yet another potential
> source of configuration errors.

Okay. As Robert pointed out the symlink is not going to work everywhere.
If the idea is agreed I'll go back to an older idea of a separate full
lookup path and some further performance optimisations.


As to why. We currently have admin who still:
 a) want to replace the entire template set with something else
 b) want to replace just some of the template set with something else
 c) want to add local deny_info ERR_* pages with alternative names

The error_directory + auto-negotiate currently caters for (a) and (c) but
not (b).

This replaces error_directory with a fixed location which can be used to
override auto-negotiate for cases (a) and (b). And also as a known location
for case (c).

My earlier idea was to let them add to errors/templates/ but some distros
need to remove or replace that folder when upgrading the langpack content.

Placing this under /etc/squid/errors.d keeps everything which is manually
configurable in one area. So the admin no longer even really needs to worry
about /usr/shared/squid/errors/ or what is in there.


> 
> Also is errors.d language sensitive?

hmm, interesting. It could be made so. With this update it is not.


> If so, is there a "default"
> language that overrides all languages?

The languages works on a back-oriented model. With the primary source
being preferred, but a series of backups available.

errors.d/ would be that override language on a per-page basis. If anything
by the wanted template name exists there it is used instead of the slow(er)
language lookups.

Currently we have two override sequences:

 if (error_directory)
   error_directory $full-path
 else
   Accept-Language: $which [, $which [...]]
   error_default_language $which
 errors/templates/* (english)


The sequence of overrides would be:
 errors.d/*
 Accept-Language: $which [, $which [...]]
 error_default_language $which
 errors/templates/* (english)


The patch here is missing a few possible performance optimizations.
 The first I intend to get in with a re-submission if this idea is
accepted. That is to add a flag to the template data loaded on
initialization to indicate whether its used unconditionally (override) or
auto-negotiate is to happen first (presently only loaded as the final
templates/* backup). This removes 1-2 cycles of file lookup on each error
request.
 Second was suggested by Adrian way back. To load the entire template set
into a memory tree and search that. This is a bit more work and requires
some performance/cost benefit analysis I don't have time for.
 Third is a smaller alternative to the second, just to cache the languages
which are available. Thus reducing some useless file probes. We have most
of the major web languages and their variants, but there are a few which
still hit two file loads.

Amos


Re: [PATCH] [RFC] custom error pages

2010-11-21 Thread Robert Collins
Also, symlinks fail on windows :(.

-Rob


Re: [PATCH] [RFC] custom error pages

2010-11-21 Thread Henrik Nordström
lör 2010-11-20 klockan 18:58 +1300 skrev Amos Jeffries:

> To simplify the error page loading a symlink is added from the templated 
> /usr/share/squid/errors/local to that /etc/squid/errors.d folder and 
> checked just before loading the errors/templates/ backup.

Why? I find having this symlink confusing, and yet another potential
source of configuration errors.

Also is errors.d language sensitive? If so, is there a "default"
language that overrides all languages?

Regards
Henrik



Re: [PATCH] [RFC] custom error pages

2010-11-21 Thread Henrik Nordström
lör 2010-11-20 klockan 18:58 +1300 skrev Amos Jeffries:

> To simplify the error page loading a symlink is added from the templated 
> /usr/share/squid/errors/local to that /etc/squid/errors.d folder and 
> checked just before loading the errors/templates/ backup.

Why? I find having this symlink confusing, and yet another potential
source of configuration errors.

Also is errors.d language sensitive? If so, is there a "default"
language that overrides all languages?

Regards
Henrik




Re: [PATCH] [RFC] custom error pages

2010-11-20 Thread Kinkie
> Due to this implementation prioritizing these templates it obsoletes
> errors_directory. Docs there are updated to deprecate it for future full
> removal.

+1

-- 
    /kinkie


[PATCH] [RFC] custom error pages

2010-11-19 Thread Amos Jeffries
It has become clear that even with the new langpack setups we need an 
area dedicated to local custom ERR_* pages which is integrated with but 
fully separate from the bundled translations.


This patch adds a configuration directory /etc/squid/errors.d/ for such 
templates to be placed.


To simplify the error page loading a symlink is added from the templated 
/usr/share/squid/errors/local to that /etc/squid/errors.d folder and 
checked just before loading the errors/templates/ backup.


Also, other ideas on what to call the symlink are welcome. I'm kind of 
partial to "local", but if anyone has a better idea we can go with that.


Due to this implementation prioritizing these templates it obsoletes 
errors_directory. Docs there are updated to deprecate it for future full 
removal.


Amos
=== modified file 'errors/Makefile.am'
--- errors/Makefile.am	2010-10-27 23:29:55 +
+++ errors/Makefile.am	2010-11-20 05:35:01 +
@@ -138,7 +138,9 @@
 		$(mkinstalldirs) $(DESTDIR)`dirname $(DEFAULT_STYLESHEET)` ; \
 		echo "$(INSTALL_DATA) $(srcdir)/errorpage.css $(DESTDIR)$(DEFAULT_STYLESHEET)"; \
 		$(INSTALL_DATA) $(srcdir)/errorpage.css $(DESTDIR)$(DEFAULT_STYLESHEET); \
-	fi
+	fi; \
+	$(mkinstalldirs) $(DESTDIR)$(sysconfdir)/errors.d; \
+	$(LN) -s $(DESTDIR)$(sysconfdir)/errors.d $(DESTDIR)$(DEFAULT_ERROR_DIR)/local
 
 install-data-local: translate
 	$(mkinstalldirs) $(DESTDIR)$(DEFAULT_ERROR_DIR) ; \
@@ -187,6 +189,7 @@
 	rm -f $(DESTDIR)$(DEFAULT_STYLESHEET).default
 	rm -f $(DESTDIR)$(DEFAULT_ERROR_DIR)/TRANSLATORS
 	rm -f $(DESTDIR)$(DEFAULT_ERROR_DIR)/COPYRIGHT
+	rm -f $(DESTDIR)$(DEFAULT_ERROR_DIR)/local
 
 ## Upgrade requires the new files to be pre-installed
 upgrade: install

=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc	2010-11-19 12:04:58 +
+++ src/cache_cf.cc	2010-11-20 05:32:19 +
@@ -698,8 +698,10 @@
 
 requirePathnameExists("Icon Directory", Config.icons.directory);
 
-if (Config.errorDirectory)
+if (Config.errorDirectory) {
+debugs(0,0, "WARNING: error_directory is deprecated. Place local error templates in /etc/squid/errors.d/");
 requirePathnameExists("Error Directory", Config.errorDirectory);
+}
 
 #if USE_HTTP_VIOLATIONS
 

=== modified file 'src/errorpage.cc'
--- src/errorpage.cc	2010-11-01 05:44:28 +
+++ src/errorpage.cc	2010-11-20 05:27:28 +
@@ -246,6 +247,10 @@
 if (Config.errorDirectory)
 text = errorTryLoadText(page_name, Config.errorDirectory);
 
+/* test custom pages location in case we have an override page */
+if (!text)
+text = errorTryLoadText(page_name, DEFAULT_SQUID_ERROR_DIR"/local");
+
 #if USE_ERR_LOCALES
 /** test error_default_language location */
 if (!text && Config.errorDefaultLanguage) {
@@ -992,6 +1016,10 @@
  */
 if (!Config.errorDirectory && page_id < ERR_MAX && request && request->header.getList(HDR_ACCEPT_LANGUAGE, &hdr) ) {
 
+// pull override from the local custom templates directory if present.
+if ((m = errorTryLoadText(err_type_str[page_id], DEFAULT_SQUID_ERROR_DIR"/local", true)))
+break;
+
 size_t pos = 0; // current parsing position in header string
 char *reset = NULL; // where to reset the p pointer for each new tag file
 char *dt = NULL;

=== modified file 'src/cf.data.pre'
--- src/cf.data.pre	2010-11-18 08:01:53 +
+++ src/cf.data.pre	2010-11-20 05:50:48 +
@@ -5805,22 +5805,13 @@
 LOC: Config.errorDirectory
 DEFAULT: none
 DOC_START
-	If you wish to create your own versions of the default
-	error files to customize them to suit your company copy
-	the error/template files to another directory and point
-	this tag at them.
-
-	WARNING: This option will disable multi-language support
-	 on error pages if used.
-
-	The squid developers are interested in making squid available in
-	a wide variety of languages. If you are making translations for a
-	language that Squid does not currently provide please consider
-	contributing your translation back to the project.
-	http://wiki.squid-cache.org/Translations
-
-	The squid developers working on translations are happy to supply drop-in
-	translated error files in exchange for any new language contributions.
+	This option was used to point Squid at the location of local
+	customized error templates. This is now deprecated.
+
+	Squid loads custom templates from /etc/squid/errors.d/
+
+	WARNING: This option will disable multi-language support on
+	 error pages if used.
 DOC_END
 
 NAME: error_default_language