Re: Intended commit ...
Curt Arnold <carnold <at> apache.org>
2006-02-28 00:18:03 GMT
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)
> <at> <at> -139,6 +139,15 <at> <at>
> LogString getProperty(const LogString&
> key) const;
>
> /**
> + Tests if this property list contains the
> specified key.
> +
> + <at> param key possible key.
> + <at> 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)
> <at> <at> -31,6 +31,10 <at> <at>
> */
> class LOG4CXX_EXPORT System
> {
> + static Properties* props;
> +
> + static void initStatic();
> +
> public:
>
> /**
> <at> <at> -45,6 +49,15 <at> <at>
> */
> static LogString getProperty(const LogString& key);
>
> + /**
> + Sets the system property indicated by the
> specified key.
> +
> + <at> param key the name of the system property.
> + <at> param value the value of the system property.
> + <at> 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)
> <at> <at> -37,65 +37,88 <at> <at>
> 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)
> <at> <at> -318,6 +318,12 <at> <at>
> 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)
> <at> <at> -0,0 +1,59 <at> <at>
> +/*
> + * 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
> + <at> author Andreas Fester
> + <at> 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)
> <at> <at> -37,6 +37,7 <at> <at>
> helpers/relativetimedateformattestcase.cpp \
> helpers/stringtokenizertestcase.cpp \
> helpers/stringhelpertestcase.cpp \
> + helpers/systemtestcase.cpp \
> helpers/timezonetestcase.cpp \
> helpers/transcodertestcase.cpp \
> helpers/unicodehelpertestcase.cpp
Okay.
Thanks & best Regards,
Andreas
> it looks like appropriate use of fwide() could ensure that
I read about that some time ago, but IIRC there were also some
portability issues. Need to have a look at it...
> 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.
I thought about that, but there is also the internal LogLog logger
which can not be configured through a ConsoleAppender parameter.
Thats why I took the "global" approach, to make sure that *all* output
is always printed through the same stream.
>> Index: include/log4cxx/helpers/properties.h
>> ===================================================================
[...]
> Looks okay, follows the JDK's Properties.containsKey()
Ok.
>> Index: include/log4cxx/helpers/system.h
>> ===================================================================
[...]
> 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;
> }
Agree.
>> Index: src/system.cpp
>> ===================================================================
[...]
> 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
Thats true. The values are written once into the appropriate system
properties.
> initialization and would not reflect changes that occur after that time.
They can be changed by the application by calling System::setProperty().
I dont know the internal implementation of the apr_()-Functions,
but is it possible that their return values (user name, temporary directory,
...) change at runtime?
Thanks,
Andreas
RSS Feed