QPID-2051: relax the parser validation to only halt startup on fatal-errors in the xml file, and relax the level-check to allow undefined system properties. Move the Log4J initialisation override inside Main instead of the startup scripts, and have it check for -Dlog4j.configuration first before engaging.

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@805188 13f79535-47bb-0310-9956-ffa450edef68

6 files changed, 97 insertions, 50 deletions

diff --git a/qpid/java/broker/bin/qpid-server b/qpid/java/broker/bin/qpid-server
index 7482ad63ef..e5a9e998e2 100755
--- a/qpid/java/broker/bin/qpid-server
+++ b/qpid/java/broker/bin/qpid-server
@@ -23,12 +23,6 @@ if [ -z "$QPID_HOME" ]; then
     export PATH=${PATH}:${QPID_HOME}/bin
 fi
 
-if [ -z "$QPID_LOG4J_SETTINGS" ]; then
-    # Disable the Log4J default initialization process, allowing the broker to 
-    # perform the configuration using etc/log4j.xml
-    QPID_LOG4J_SETTINGS="-Dlog4j.defaultInitOverride=true"
-fi
-
 # Set classpath to include Qpid jar with all required jars in manifest
 QPID_LIBS=$QPID_HOME/lib/qpid-all.jar:$QPID_HOME/lib/bdbstore-launch.jar
 
@@ -40,6 +34,6 @@ export JAVA=java \
        QPID_CLASSPATH=$QPID_LIBS \
        QPID_RUN_LOG=2
 
-QPID_OPTS="$QPID_OPTS $QPID_LOG4J_SETTINGS -Damqj.read_write_pool_size=32"
+QPID_OPTS="$QPID_OPTS -Damqj.read_write_pool_size=32"
 
 . qpid-run org.apache.qpid.server.Main "$@"
diff --git a/qpid/java/broker/bin/qpid-server.bat b/qpid/java/broker/bin/qpid-server.bat
index 7fff183192..2687baa111 100644
--- a/qpid/java/broker/bin/qpid-server.bat
+++ b/qpid/java/broker/bin/qpid-server.bat
@@ -20,9 +20,6 @@
 @echo off

 REM Script to run the Qpid Java Broker

 

-rem stop the Log4J default initialisation, let the broker do it using etc/log4j.xml

-if "%QPID_LOG4J_SETTINGS%" == "" set QPID_LOG4J_SETTINGS=-Dlog4j.defaultInitOverride=true

-

 rem Guess QPID_HOME if not defined

 set CURRENT_DIR=%cd%

 if not "%QPID_HOME%" == "" goto gotHome

@@ -197,7 +194,7 @@ rem QPID_OPTS intended to hold any -D props for use
 rem user must enclose any value for QPID_OPTS in double quotes

 :runCommand

 set MODULE_JARS=%QPID_MODULE_JARS%

-set COMMAND="%JAVA_HOME%\bin\java" %JAVA_VM% %JAVA_MEM% %JAVA_GC% %QPID_OPTS% %QPID_LOG4J_SETTINGS% %SYSTEM_PROPS% -cp "%CLASSPATH%;%MODULE_JARS%" org.apache.qpid.server.Main %QPID_ARGS%

+set COMMAND="%JAVA_HOME%\bin\java" %JAVA_VM% %JAVA_MEM% %JAVA_GC% %QPID_OPTS% %SYSTEM_PROPS% -cp "%CLASSPATH%;%MODULE_JARS%" org.apache.qpid.server.Main %QPID_ARGS%

 

 if "%debug%" == "true" echo %CLASSPATH%;%LAUNCH_JAR%;%MODULE_JARS%

 if "%debug%" == "true" echo %COMMAND%

diff --git a/qpid/java/broker/src/main/java/org/apache/log4j/xml/QpidLog4JConfigurator.java b/qpid/java/broker/src/main/java/org/apache/log4j/xml/QpidLog4JConfigurator.java
index aa6694b3b1..4afc209ba7 100644
--- a/qpid/java/broker/src/main/java/org/apache/log4j/xml/QpidLog4JConfigurator.java
+++ b/qpid/java/broker/src/main/java/org/apache/log4j/xml/QpidLog4JConfigurator.java
@@ -32,9 +32,9 @@ import javax.xml.parsers.ParserConfigurationException;
 import org.apache.log4j.Level;
 import org.apache.log4j.Logger;
 import org.apache.qpid.server.logging.management.LoggingManagementMBean;
-import org.apache.qpid.server.logging.management.LoggingManagementMBean.QpidLog4JSaxErrorHandler;
 import org.xml.sax.ErrorHandler;
 import org.xml.sax.SAXException;
+import org.xml.sax.SAXParseException;
 
 /**
  * Substitute for the Log4J XMLWatchdog (as used by DOMConfigurator.configureAndWatch)
@@ -50,6 +50,7 @@ public class QpidLog4JConfigurator
     //shared with LoggingManagementMBean
     public static final ReentrantLock LOCK = new ReentrantLock();
     private static Logger _logger;
+    private static DOMConfigurator domConfig = new DOMConfigurator();
 
     private QpidLog4JConfigurator()
     {
@@ -63,10 +64,15 @@ public class QpidLog4JConfigurator
         {
             LOCK.lock();
 
-            strictlyParseXMLConfigFile(filename);
+            parseXMLConfigFile(filename);
             checkLoggerLevels(filename);
 
             DOMConfigurator.configure(filename);
+            
+            if(_logger == null)
+            {
+                _logger = Logger.getLogger(QpidLog4JConfigurator.class);
+            }
         }
         finally
         {
@@ -77,7 +83,7 @@ public class QpidLog4JConfigurator
     public static void configureAndWatch(String filename, long delay) throws IOException, ParserConfigurationException, 
                                                                              SAXException, IllegalLoggerLevelException
     {
-        strictlyParseXMLConfigFile(filename);
+        parseXMLConfigFile(filename);
         checkLoggerLevels(filename);
         
         QpidLog4JXMLWatchdog watchdog = new QpidLog4JXMLWatchdog(filename);
@@ -85,7 +91,7 @@ public class QpidLog4JConfigurator
         watchdog.start();
     }
     
-    private static void strictlyParseXMLConfigFile(String fileName) throws IOException, SAXException,
+    private static void parseXMLConfigFile(String fileName) throws IOException, SAXException,
                                                                                         ParserConfigurationException
     {
         try
@@ -127,6 +133,43 @@ public class QpidLog4JConfigurator
         }
     }
     
+    public static class QpidLog4JSaxErrorHandler implements ErrorHandler
+    {
+        public void error(SAXParseException e) throws SAXException
+        {
+            if(_logger != null)
+            {
+                _logger.warn(constructMessage("Error parsing XML file", e));
+            }
+            else
+            {
+                System.err.println(constructMessage("Error parsing XML file", e));
+            }
+        }
+
+        public void fatalError(SAXParseException e) throws SAXException
+        {
+            throw new SAXException(constructMessage("Fatal error parsing XML file", e));
+        }
+
+        public void warning(SAXParseException e) throws SAXException
+        {
+            if(_logger != null)
+            {
+                _logger.warn(constructMessage("Warning parsing XML file", e));
+            }
+            else
+            {
+                System.err.println(constructMessage("Warning parsing XML file", e));
+            }
+        }
+
+        private static String constructMessage(final String msg, final SAXParseException ex)
+        {
+            return new String(msg + ": Line " + ex.getLineNumber()+" column " +ex.getColumnNumber() + ": " + ex.getMessage());
+        }
+    }
+    
     private static class QpidLog4JXMLWatchdog extends XMLWatchdog
     {
         public QpidLog4JXMLWatchdog(String filename)
@@ -142,7 +185,7 @@ public class QpidLog4JConfigurator
                 
                 try
                 {
-                    strictlyParseXMLConfigFile(filename);
+                    parseXMLConfigFile(filename);
                 }
                 catch (Exception e)
                 {
@@ -201,18 +244,43 @@ public class QpidLog4JConfigurator
         {
             LOCK.lock();
             
+            //get the Logger levels to check
             Map<String, String> loggersLevels;
             loggersLevels = LoggingManagementMBean.retrieveConfigFileLoggersLevels(filename);
+            //add the RootLogger to the list too
+            String rootLoggerlevelString = LoggingManagementMBean.retrieveConfigFileRootLoggerLevel(filename);
+            loggersLevels.put("Root", rootLoggerlevelString);
+            
 
             for (String loggerName : loggersLevels.keySet())
             {
                 String levelString = loggersLevels.get(loggerName);
-                checkLevel(loggerName,levelString);
+                
+                //let log4j replace any properties in the string
+                String log4jConfiguredString = domConfig.subst(levelString);
+                
+                if(log4jConfiguredString.equals("") & ! log4jConfiguredString.equals(levelString))
+                {
+                    //log4j has returned an empty string but this isnt what we gave it.
+                    //There may have been an undefined property. Unlike an incorrect
+                    //literal value, we will allow this case to proceed, but warn users.
+                    
+                    if(_logger != null)
+                    {
+                        _logger.warn("Unable to detect Level value from '" + levelString 
+                                +"' for logger '" + loggerName + "', Log4J will default this to DEBUG");
+                    }
+                    else
+                    {
+                        System.err.println("Unable to detect Level value from '" + levelString 
+                                +"' for logger " + loggerName + ", Log4J will default this to DEBUG");
+                    }
+                    
+                    continue;
+                }
+                
+                checkLevel(loggerName,log4jConfiguredString);
             }
-
-            //check the root logger level
-            String rootLoggerlevelString = LoggingManagementMBean.retrieveConfigFileRootLoggerLevel(filename);
-            checkLevel("Root", rootLoggerlevelString);
         }
         finally
         {
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/Main.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/Main.java
index 8566ba6270..f8deb95628 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/Main.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/Main.java
@@ -69,8 +69,8 @@ import java.util.Properties;
 @SuppressWarnings({"AccessStaticViaInstance"})
 public class Main
 {
-    private static final Logger _logger = Logger.getLogger(Main.class);
-    public static final Logger _brokerLogger = Logger.getLogger("Qpid.Broker");
+    private static Logger _logger;
+    private static Logger _brokerLogger;
 
     private static final String DEFAULT_CONFIG_FILE = "etc/config.xml";
 
@@ -468,7 +468,18 @@ public class Main
 
     public static void main(String[] args)
     {
-
+        //if the -Dlog4j.configuration property has not been set, enable the init override
+        //to stop Log4J wondering off and picking up the first log4j.xml/properties file it
+        //finds from the classpath when we get the first Loggers
+        if(System.getProperty("log4j.configuration") == null)
+        {
+            System.setProperty("log4j.defaultInitOverride", "true");
+        }
+        
+        //now that the override status is know, we can instantiate the Loggers
+        _logger = Logger.getLogger(Main.class);
+        _brokerLogger = Logger.getLogger("Qpid.Broker");
+        
         new Main(args);
     }
 
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/management/LoggingManagementMBean.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/management/LoggingManagementMBean.java
index d98b14e4d5..3c47cdd094 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/management/LoggingManagementMBean.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/management/LoggingManagementMBean.java
@@ -40,6 +40,7 @@ import org.apache.log4j.LogManager;
 import org.apache.log4j.Logger;
 import org.apache.log4j.xml.Log4jEntityResolver;
 import org.apache.log4j.xml.QpidLog4JConfigurator;
+import org.apache.log4j.xml.QpidLog4JConfigurator.QpidLog4JSaxErrorHandler;
 import org.apache.log4j.xml.QpidLog4JConfigurator.IllegalLoggerLevelException;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
@@ -266,30 +267,6 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
         return newLevel;
     }
     
-    //handler to catch errors signalled by the JAXP parser and throw an appropriate exception
-    public static class QpidLog4JSaxErrorHandler implements ErrorHandler
-    {
-        public void error(SAXParseException e) throws SAXException
-        {
-            throw new SAXException(constructMessage("Error parsing XML file", e));
-        }
-
-        public void fatalError(SAXParseException e) throws SAXException
-        {
-            throw new SAXException(constructMessage("Fatal error parsing XML file", e));
-        }
-
-        public void warning(SAXParseException e) throws SAXException
-        {
-            throw new SAXException(constructMessage("Warning parsing XML file", e));
-        }
-
-        private static String constructMessage(final String msg, final SAXParseException ex)
-        {
-            return new String(msg + ": Line " + ex.getLineNumber()+" column " +ex.getColumnNumber() + ": " + ex.getMessage());
-        }
-    }
-
     //method to parse the XML configuration file, validating it in the process, and returning a DOM Document of the content.
     private static synchronized Document parseConfigFile(String fileName) throws IOException
     {
diff --git a/qpid/java/broker/src/test/java/org/apache/log4j/xml/QpidLog4JConfiguratorTest.java b/qpid/java/broker/src/test/java/org/apache/log4j/xml/QpidLog4JConfiguratorTest.java
index 643f1fa48e..9f350033d6 100644
--- a/qpid/java/broker/src/test/java/org/apache/log4j/xml/QpidLog4JConfiguratorTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/log4j/xml/QpidLog4JConfiguratorTest.java
@@ -386,11 +386,11 @@ public class QpidLog4JConfiguratorTest extends TestCase
         }
         catch (IllegalLoggerLevelException e)
         {
-            fail("Incorrect Exception, expected an IOException");
+            //expected, ignore
         }
         catch (IOException e)
         {
-            //expected, ignore
+            fail("Incorrect Exception, expected an IllegalLoggerLevelException");
         }
     }
 }