On Feb 27, 2006, at 3:12 PM, Andreas Fester wrote:
I indent to commit the attached patch. It adds the
possibility to set system properties (they are
still retrieved from the environment if they do not exist yet
when they are read the first time).
This will allow applications to set any kind of system
property to configure very basic behaviour of log4cxx.
Its primarily a preparation for LOGCXX-126:
it will allow to set a system property like "log4cxx.useWideStreams"
very early in the application to switch to wide streams output
(We need a runtime configuration there; see my comment
at http://issues.apache.org/jira/browse/LOGCXX-126#action_12367428).
I'll add comments back to that bug report. I'm scrambling on this,
but it looks like appropriate use of fwide() could ensure that
ConsoleAppender uses the correct mode if somebody else already wrote
to stdout before the first call to ConsoleAppender. Using a system
property to control this is down on my list of ways to approach the
problem, I'd rather had configuration parameters or a distinct
WideConsoleAppender before depending on a system property.
Comments on the proposed patch:
Thanks,
Andreas
Index: include/log4cxx/helpers/properties.h
===================================================================
--- include/log4cxx/helpers/properties.h (Revision 378958)
+++ include/log4cxx/helpers/properties.h (Arbeitskopie)
@@ -139,6 +139,15 @@
LogString getProperty(const LogString&
key) const;
/**
+ Tests if this property list contains the
specified key.
+
+ @param key possible key.
+ @return <code>true</code> if this
property list contains the
+ specified key, <code>false</code>
otherwise.
+ */
+ bool containsKey(const LogString& key);
+
+ /**
Returns an enumeration of all the keys in
this property list,
including distinct keys in the default
property list if a key
of the same name has not already been
found from the main
Looks okay, follows the JDK's Properties.containsKey()
Index: include/log4cxx/helpers/system.h
===================================================================
--- include/log4cxx/helpers/system.h (Revision 378958)
+++ include/log4cxx/helpers/system.h (Arbeitskopie)
@@ -31,6 +31,10 @@
*/
class LOG4CXX_EXPORT System
{
+ static Properties* props;
+
+ static void initStatic();
+
public:
/**
@@ -45,6 +49,15 @@
*/
static LogString getProperty(const LogString& key);
+ /**
+ Sets the system property indicated by the
specified key.
+
+ @param key the name of the system property.
+ @param value the value of the system property.
+ @return the previous value of the system
property or an empty string
+ if it did not have one.
+ */
+ static LogString setProperty(const LogString& key,
const LogString& value);
};
} // namespace helpers
} // namespace log4cxx
Been trying to avoid non-local static variables. In this case, I
doubt that props is ever reclaimed. I'd prefer something like:
static PropertiesPtr getProperties() {
static PropertiesPtr properties(new Properties());
return properties;
}
Index: src/system.cpp
===================================================================
--- src/system.cpp (Revision 378958)
+++ src/system.cpp (Arbeitskopie)
@@ -37,65 +37,88 @@
using namespace log4cxx::helpers;
-LogString System::getProperty(const LogString& lkey)
-{
- if (lkey.empty())
- {
- throw IllegalArgumentException("key is empty");
+Properties* System::props;
+
+void System::initStatic() {
+ static bool isInitialized = false;
+ if (isInitialized) {
+ return;
}
+ isInitialized = true;
- LogString rv;
- if (lkey == LOG4CXX_STR("java.io.tmpdir")) {
- Pool p;
- const char* dir = NULL;
- apr_status_t stat = apr_temp_dir_get(&dir, (apr_pool_t*)
p.getAPRPool());
- if (stat == APR_SUCCESS) {
- Transcoder::decode(dir, strlen(dir), rv);
- }
- return rv;
+ // system properties live until the application terminates
+ props = new Properties();
+
+ Pool pool;
+ apr_pool_t* p = (apr_pool_t*) pool.getAPRPool();
+
+ // java.io.tmpdir
+ LogString tempDir;
+ const char* dir = NULL;
+ apr_status_t stat = apr_temp_dir_get(&dir, p);
+ if (stat == APR_SUCCESS) {
+ Transcoder::decode(dir, strlen(dir), tempDir);
}
+ props->setProperty(LOG4CXX_STR("java.io.tmpdir"), tempDir);
- if (lkey == LOG4CXX_STR("user.dir")) {
- Pool p;
- char* dir = NULL;
- apr_status_t stat = apr_filepath_get(&dir,
APR_FILEPATH_NATIVE,
- (apr_pool_t*) p.getAPRPool());
- if (stat == APR_SUCCESS) {
- Transcoder::decode(dir, strlen(dir), rv);
- }
- return rv;
+ // user.dir
+ char* userdir = NULL;
+ LogString userDir;
+ stat = apr_filepath_get(&userdir, APR_FILEPATH_NATIVE, p);
+ if (stat == APR_SUCCESS) {
+ Transcoder::decode(userdir, strlen(userdir), userDir);
}
+ props->setProperty(LOG4CXX_STR("user.dir"), userDir);
+
+ // user.home
+ // user.name
#if APR_HAS_USER
- if (lkey == LOG4CXX_STR("user.home") || lkey == LOG4CXX_STR
("user.name")) {
- Pool pool;
- apr_uid_t userid;
- apr_gid_t groupid;
- apr_pool_t* p = (apr_pool_t*) pool.getAPRPool();
- apr_status_t stat = apr_uid_current(&userid, &groupid, p);
+ LogString userName;
+ LogString userHome;
+
+ apr_uid_t userid;
+ apr_gid_t groupid;
+ stat = apr_uid_current(&userid, &groupid, p);
+ if (stat == APR_SUCCESS) {
+ char* username = NULL;
+ stat = apr_uid_name_get(&username, userid, p);
if (stat == APR_SUCCESS) {
- char* username = NULL;
- stat = apr_uid_name_get(&username, userid, p);
+ Transcoder::decode(username, strlen(username), userName);
+
+ char* dirname = NULL;
+ stat = apr_uid_homepath_get(&dirname, username, p);
if (stat == APR_SUCCESS) {
- if (lkey == LOG4CXX_STR("user.name")) {
- Transcoder::decode(username, strlen(username), rv);
- } else {
- char* dirname = NULL;
- stat = apr_uid_homepath_get(&dirname, username, p);
- if (stat == APR_SUCCESS) {
- Transcoder::decode(dirname, strlen(dirname), rv);
- }
- }
+ Transcoder::decode(dirname, strlen(dirname), userHome);
}
}
- return rv;
}
+
+ props->setProperty(LOG4CXX_STR("user.home"), userHome);
+ props->setProperty(LOG4CXX_STR("user.name"), userName);
#endif
+}
- LOG4CXX_ENCODE_CHAR(key, lkey);
- const char * value = ::getenv(key.c_str());
- if (value != 0) {
- Transcoder::decode(value, rv);
+
+LogString System::getProperty(const LogString& lkey)
+{
+ initStatic();
+
+ // if the key is not yet in the properties, then add it
+ if (!props->containsKey(lkey)) {
+ LogString envVar;
+ LOG4CXX_ENCODE_CHAR(key, lkey);
+ const char * value = ::getenv(key.c_str());
+ if (value != 0) {
+ Transcoder::decode(value, envVar);
+ }
+ props->setProperty(lkey, envVar);
}
- return rv;
+
+ return props->getProperty(lkey);
}
+
+LogString System::setProperty(const LogString& key, const
LogString& value) {
+ initStatic();
+ return props->setProperty(key, value);
+}
I could be misinterpreting this code since I'm looking at the patch
itself and not the patched code. It looks like your patch would
likely cache the values of java.io.tmpdir, user.home, environment at
initialization and would not reflect changes that occur after that time.
Index: src/properties.cpp
===================================================================
--- src/properties.cpp (Revision 378958)
+++ src/properties.cpp (Arbeitskopie)
@@ -318,6 +318,12 @@
return oldValue;
}
+bool Properties::containsKey(const LogString& key)
+{
+ std::map<LogString, LogString>::const_iterator it =
properties.find(key);
+ return (it != properties.end());
+}
+
LogString Properties::getProperty(const LogString& key) const
{
std::map<LogString, LogString>::const_iterator it =
properties.find(key);
Okay.
Index: tests/src/helpers/systemtestcase.cpp
===================================================================
--- tests/src/helpers/systemtestcase.cpp (Revision 0)
+++ tests/src/helpers/systemtestcase.cpp (Revision 0)
@@ -0,0 +1,59 @@
+/*
+ * Copyright 2005 The Apache Software Foundation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
+ * See the License for the specific language governing permissions
and
+ * limitations under the License.
+ */
+
+#include <log4cxx/helpers/system.h>
+#include <cppunit/extensions/HelperMacros.h>
+
+#include <log4cxx/helpers/loglog.h>
+
+using namespace log4cxx;
+using namespace log4cxx::helpers;
+
+/**
+ Unit tests for the System class
+ @author Andreas Fester
+ @since 0.9.8
+*/
+class SystemTestCase : public CppUnit::TestFixture {
+
+ CPPUNIT_TEST_SUITE( SystemTestCase );
+ CPPUNIT_TEST( testSystemProperties );
+ CPPUNIT_TEST_SUITE_END();
+
+public:
+ /**
+ * Check that System::getProperty() returns some values
+ */
+ void testSystemProperties() {
+ // java.io.tmpdir and user.dir always exist
+ LogString tmpDir = System::getProperty("java.io.tmpdir");
+ CPPUNIT_ASSERT(!tmpDir.empty());
+
+ LogString usrDir = System::getProperty("user.dir");
+ CPPUNIT_ASSERT(!usrDir.empty());
+
+ // check propagation of environment variables
+ LogString totoValue = System::getProperty("TOTO");
+ CPPUNIT_ASSERT(totoValue == "wonderful");
+
+ // check unknown property
+ LogString unknown = System::getProperty("unknown.property");
+ CPPUNIT_ASSERT(unknown.empty());
+ }
+};
+
Okay.
+
+CPPUNIT_TEST_SUITE_REGISTRATION(SystemTestCase);
Index: tests/src/Makefile.am
===================================================================
--- tests/src/Makefile.am (Revision 378958)
+++ tests/src/Makefile.am (Arbeitskopie)
@@ -37,6 +37,7 @@
helpers/relativetimedateformattestcase.cpp \
helpers/stringtokenizertestcase.cpp \
helpers/stringhelpertestcase.cpp \
+ helpers/systemtestcase.cpp \
helpers/timezonetestcase.cpp \
helpers/transcodertestcase.cpp \
helpers/unicodehelpertestcase.cpp
Okay.
