LOGCXX-464: TimeBasedRollingPolicy::initialize was able to forward an "append" property from the caller appender, the rollover method instead wasn't and originally used a hard coded value of "false", which was later enhanced in LOGCXX-412 to true/false depending on some macro. This looks like an error with the API itself to me so I changed rollover to need an append property as well. Originally I thought of creating a backwards compatible wrapper still providing a hard coded value of true/false depending on the macro, but because of the abstract base RollingPolicy I needed to change that and all children anyways. While the current approach might break callers implementing their own policy, to me this looks like the better approach because that way those implementers need to think of the original error and act accordingly. LOGCXX-412 shows that some users already encountered the same error and resolved it with just another hard coded value.
Project: http://git-wip-us.apache.org/repos/asf/logging-log4cxx/repo Commit: http://git-wip-us.apache.org/repos/asf/logging-log4cxx/commit/6854cd00 Tree: http://git-wip-us.apache.org/repos/asf/logging-log4cxx/tree/6854cd00 Diff: http://git-wip-us.apache.org/repos/asf/logging-log4cxx/diff/6854cd00 Branch: refs/heads/master Commit: 6854cd0056449baf4425146a8fbf319b328f077a Parents: 5e33d7b Author: Thorsten Schöning <[email protected]> Authored: Tue Apr 5 15:48:18 2016 +0000 Committer: Thorsten Schöning <[email protected]> Committed: Tue Apr 5 15:48:18 2016 +0000 ---------------------------------------------------------------------- src/main/cpp/fixedwindowrollingpolicy.cpp | 122 +++++++++++-------- src/main/cpp/rollingfileappender.cpp | 22 ++-- src/main/cpp/timebasedrollingpolicy.cpp | 43 +++---- .../log4cxx/rolling/fixedwindowrollingpolicy.h | 46 +++---- .../include/log4cxx/rolling/rollingpolicy.h | 64 +++++----- .../log4cxx/rolling/timebasedrollingpolicy.h | 44 +++---- 6 files changed, 164 insertions(+), 177 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/logging-log4cxx/blob/6854cd00/src/main/cpp/fixedwindowrollingpolicy.cpp ---------------------------------------------------------------------- diff --git a/src/main/cpp/fixedwindowrollingpolicy.cpp b/src/main/cpp/fixedwindowrollingpolicy.cpp index 1a6b5d6..57f71aa 100644 --- a/src/main/cpp/fixedwindowrollingpolicy.cpp +++ b/src/main/cpp/fixedwindowrollingpolicy.cpp @@ -97,19 +97,22 @@ void FixedWindowRollingPolicy::activateOptions(Pool& p) { * {@inheritDoc} */ RolloverDescriptionPtr FixedWindowRollingPolicy::initialize( - const LogString& file, bool append, log4cxx::helpers::Pool& p) { - LogString newActiveFile(file); + const LogString& currentActiveFile, + const bool append, + Pool& pool) +{ + LogString newActiveFile(currentActiveFile); explicitActiveFile = false; - if (file.length() > 0) { + if (currentActiveFile.length() > 0) { explicitActiveFile = true; - newActiveFile = file; + newActiveFile = currentActiveFile; } if (!explicitActiveFile) { LogString buf; ObjectPtr obj(new Integer(minIndex)); - formatFileName(obj, buf, p); + formatFileName(obj, buf, pool); newActiveFile = buf; } @@ -122,54 +125,67 @@ RolloverDescriptionPtr FixedWindowRollingPolicy::initialize( * {@inheritDoc} */ RolloverDescriptionPtr FixedWindowRollingPolicy::rollover( - const LogString& currentFileName, - log4cxx::helpers::Pool& p) { - RolloverDescriptionPtr desc; - if (maxIndex >= 0) { - int purgeStart = minIndex; - - if (!explicitActiveFile) { - purgeStart++; - } - - if (!purge(purgeStart, maxIndex, p)) { - return desc; - } - - LogString buf; - ObjectPtr obj(new Integer(purgeStart)); - formatFileName(obj, buf, p); - - LogString renameTo(buf); - LogString compressedName(renameTo); - ActionPtr compressAction ; - - if (StringHelper::endsWith(renameTo, LOG4CXX_STR(".gz"))) { - renameTo.resize(renameTo.size() - 3); - compressAction = - new GZCompressAction( - File().setPath(renameTo), File().setPath(compressedName), true); - } else if (StringHelper::endsWith(renameTo, LOG4CXX_STR(".zip"))) { - renameTo.resize(renameTo.size() - 4); - compressAction = - new ZipCompressAction( - File().setPath(renameTo), File().setPath(compressedName), true); - } - - FileRenameActionPtr renameAction = - new FileRenameAction( - File().setPath(currentFileName), File().setPath(renameTo), false); - -#ifdef LOG4CXX_MULTI_PROCESS - desc = new RolloverDescription( - currentFileName, true, renameAction, compressAction); -#else - desc = new RolloverDescription( - currentFileName, false, renameAction, compressAction); -#endif - } - - return desc; + const LogString& currentActiveFile, + const bool append, + Pool& pool) +{ + RolloverDescriptionPtr desc; + + if (maxIndex < 0) + { + return desc; + } + + int purgeStart = minIndex; + + if (!explicitActiveFile) + { + purgeStart++; + } + + if (!purge(purgeStart, maxIndex, pool)) + { + return desc; + } + + LogString buf; + ObjectPtr obj(new Integer(purgeStart)); + formatFileName(obj, buf, pool); + + LogString renameTo(buf); + LogString compressedName(renameTo); + ActionPtr compressAction ; + + if (StringHelper::endsWith(renameTo, LOG4CXX_STR(".gz"))) + { + renameTo.resize(renameTo.size() - 3); + compressAction = + new GZCompressAction( + File().setPath(renameTo), + File().setPath(compressedName), + true); + } + else if (StringHelper::endsWith(renameTo, LOG4CXX_STR(".zip"))) + { + renameTo.resize(renameTo.size() - 4); + compressAction = + new ZipCompressAction( + File().setPath(renameTo), + File().setPath(compressedName), + true); + } + + FileRenameActionPtr renameAction = + new FileRenameAction( + File().setPath(currentActiveFile), + File().setPath(renameTo), + false); + + desc = new RolloverDescription( + currentActiveFile, append, + renameAction, compressAction); + + return desc; } /** http://git-wip-us.apache.org/repos/asf/logging-log4cxx/blob/6854cd00/src/main/cpp/rollingfileappender.cpp ---------------------------------------------------------------------- diff --git a/src/main/cpp/rollingfileappender.cpp b/src/main/cpp/rollingfileappender.cpp index ddbebd5..777d0f8 100644 --- a/src/main/cpp/rollingfileappender.cpp +++ b/src/main/cpp/rollingfileappender.cpp @@ -168,7 +168,7 @@ bool RollingFileAppenderSkeleton::rollover(Pool& p) { #ifdef LOG4CXX_MULTI_PROCESS std::string fileName(getFile()); - RollingPolicyBase *basePolicy = dynamic_cast<RollingPolicyBase* >(&(*rollingPolicy)); + RollingPolicyBase *basePolicy = dynamic_cast<RollingPolicyBase* >(&(*rollingPolicy)); apr_time_t n = apr_time_now(); ObjectPtr obj(new Date(n)); LogString fileNamePattern; @@ -185,7 +185,7 @@ bool RollingFileAppenderSkeleton::rollover(Pool& p) { char szUid[MAX_FILE_LEN] = {'\0'}; memcpy(szDirName, fileName.c_str(), fileName.size() > MAX_FILE_LEN ? MAX_FILE_LEN : fileName.size()); memcpy(szBaseName, fileName.c_str(), fileName.size() > MAX_FILE_LEN ? MAX_FILE_LEN : fileName.size()); - apr_uid_t uid; + apr_uid_t uid; apr_gid_t groupid; apr_status_t stat = apr_uid_current(&uid, &groupid, pool.getAPRPool()); if (stat == APR_SUCCESS){ @@ -218,7 +218,7 @@ bool RollingFileAppenderSkeleton::rollover(Pool& p) { if (bAlreadyRolled){ apr_finfo_t finfo1, finfo2; apr_status_t st1, st2; - apr_file_t* _fd = getWriter()->getOutPutStreamPtr()->getFileOutPutStreamPtr().getFilePtr(); + apr_file_t* _fd = getWriter()->getOutPutStreamPtr()->getFileOutPutStreamPtr().getFilePtr(); st1 = apr_file_info_get(&finfo1, APR_FINFO_IDENT, _fd); if (st1 != APR_SUCCESS){ LogLog::warn(LOG4CXX_STR("apr_file_info_get failed")); @@ -229,14 +229,14 @@ bool RollingFileAppenderSkeleton::rollover(Pool& p) { LogLog::warn(LOG4CXX_STR("apr_stat failed.")); } - bAlreadyRolled = ((st1 == APR_SUCCESS) && (st2 == APR_SUCCESS) + bAlreadyRolled = ((st1 == APR_SUCCESS) && (st2 == APR_SUCCESS) && ((finfo1.device != finfo2.device) || (finfo1.inode != finfo2.inode))); } - + if (!bAlreadyRolled){ #endif try { - RolloverDescriptionPtr rollover1(rollingPolicy->rollover(getFile(), p)); + RolloverDescriptionPtr rollover1(rollingPolicy->rollover(this->getFile(), this->getAppend(), p)); if (rollover1 != NULL) { if (rollover1->getActiveFileName() == getFile()) { closeWriter(); @@ -337,7 +337,7 @@ bool RollingFileAppenderSkeleton::rollover(Pool& p) { * re-open current file when its own handler has been renamed */ void RollingFileAppenderSkeleton::reopenLatestFile(Pool& p){ - closeWriter(); + closeWriter(); OutputStreamPtr os(new FileOutputStream(getFile(), true)); WriterPtr newWriter(createWriter(os)); setFile(getFile()); @@ -371,11 +371,11 @@ void RollingFileAppenderSkeleton::subAppend(const LoggingEventPtr& event, Pool& } #ifdef LOG4CXX_MULTI_PROCESS - //do re-check before every write + //do re-check before every write // apr_finfo_t finfo1, finfo2; apr_status_t st1, st2; - apr_file_t* _fd = getWriter()->getOutPutStreamPtr()->getFileOutPutStreamPtr().getFilePtr(); + apr_file_t* _fd = getWriter()->getOutPutStreamPtr()->getFileOutPutStreamPtr().getFilePtr(); st1 = apr_file_info_get(&finfo1, APR_FINFO_IDENT, _fd); if (st1 != APR_SUCCESS){ LogLog::warn(LOG4CXX_STR("apr_file_info_get failed")); @@ -387,7 +387,7 @@ void RollingFileAppenderSkeleton::subAppend(const LoggingEventPtr& event, Pool& LogLog::warn(LOG4CXX_STR(err.c_str())); } - bool bAlreadyRolled = ((st1 == APR_SUCCESS) && (st2 == APR_SUCCESS) + bool bAlreadyRolled = ((st1 == APR_SUCCESS) && (st2 == APR_SUCCESS) && ((finfo1.device != finfo2.device) || (finfo1.inode != finfo2.inode))); if (bAlreadyRolled){ @@ -524,7 +524,7 @@ size_t RollingFileAppenderSkeleton::getFileLength() const { return fileLength; } -#ifdef LOG4CXX_MULTI_PROCESS +#ifdef LOG4CXX_MULTI_PROCESS void RollingFileAppenderSkeleton::setFileLength(size_t length){ fileLength = length; } http://git-wip-us.apache.org/repos/asf/logging-log4cxx/blob/6854cd00/src/main/cpp/timebasedrollingpolicy.cpp ---------------------------------------------------------------------- diff --git a/src/main/cpp/timebasedrollingpolicy.cpp b/src/main/cpp/timebasedrollingpolicy.cpp index 974b00f..0f786ec 100644 --- a/src/main/cpp/timebasedrollingpolicy.cpp +++ b/src/main/cpp/timebasedrollingpolicy.cpp @@ -18,9 +18,9 @@ #pragma warning ( disable: 4231 4251 4275 4786 ) #endif -#ifdef LOG4CXX_MULTI_PROCESS +#ifdef LOG4CXX_MULTI_PROCESS #include <libgen.h> -#endif +#endif #include <log4cxx/logstring.h> #include <log4cxx/rolling/timebasedrollingpolicy.h> @@ -47,10 +47,10 @@ using namespace log4cxx::pattern; IMPLEMENT_LOG4CXX_OBJECT(TimeBasedRollingPolicy) -#ifdef LOG4CXX_MULTI_PROCESS +#ifdef LOG4CXX_MULTI_PROCESS #define MMAP_FILE_SUFFIX ".map" #define LOCK_FILE_SUFFIX ".maplck" -#define MAX_FILE_LEN 2048 +#define MAX_FILE_LEN 2048 bool TimeBasedRollingPolicy::isMapFileEmpty(log4cxx::helpers::Pool& pool){ apr_finfo_t finfo; @@ -83,7 +83,7 @@ const std::string TimeBasedRollingPolicy::createFile(const std::string& fileName memcpy(szDirName, fileName.c_str(), fileName.size() > MAX_FILE_LEN ? MAX_FILE_LEN : fileName.size()); memcpy(szBaseName, fileName.c_str(), fileName.size() > MAX_FILE_LEN ? MAX_FILE_LEN : fileName.size()); - apr_uid_t uid; + apr_uid_t uid; apr_gid_t groupid; apr_status_t stat = apr_uid_current(&uid, &groupid, pool.getAPRPool()); if (stat == APR_SUCCESS){ @@ -138,14 +138,14 @@ int TimeBasedRollingPolicy::unLockMMapFile() #endif -TimeBasedRollingPolicy::TimeBasedRollingPolicy() -#ifdef LOG4CXX_MULTI_PROCESS +TimeBasedRollingPolicy::TimeBasedRollingPolicy() +#ifdef LOG4CXX_MULTI_PROCESS :_mmap(NULL), _file_map(NULL), bAlreadyInitialized(false), _mmapPool(new Pool()), _lock_file(NULL), bRefreshCurFile(false) #endif { } -#ifdef LOG4CXX_MULTI_PROCESS +#ifdef LOG4CXX_MULTI_PROCESS TimeBasedRollingPolicy::~TimeBasedRollingPolicy() { //no-need to delete mmap delete _mmapPool; @@ -196,7 +196,7 @@ void TimeBasedRollingPolicy::activateOptions(log4cxx::helpers::Pool& pool) { LogLog::warn(LOG4CXX_STR("open lock file failed.")); } } - + initMMapFile(lastFileName, *_mmapPool); #endif @@ -226,9 +226,10 @@ log4cxx::pattern::PatternMap TimeBasedRollingPolicy::getFormatSpecifiers() const * {@inheritDoc} */ RolloverDescriptionPtr TimeBasedRollingPolicy::initialize( - const LogString& currentActiveFile, - const bool append, - Pool& pool) { + const LogString& currentActiveFile, + const bool append, + Pool& pool) +{ apr_time_t n = apr_time_now(); nextCheck = ((n / APR_USEC_PER_SEC) + 1) * APR_USEC_PER_SEC; @@ -252,11 +253,11 @@ RolloverDescriptionPtr TimeBasedRollingPolicy::initialize( } } - - RolloverDescriptionPtr TimeBasedRollingPolicy::rollover( - const LogString& currentActiveFile, - Pool& pool) { + const LogString& currentActiveFile, + const bool append, + Pool& pool) +{ apr_time_t n = apr_time_now(); nextCheck = ((n / APR_USEC_PER_SEC) + 1) * APR_USEC_PER_SEC; @@ -331,17 +332,9 @@ RolloverDescriptionPtr TimeBasedRollingPolicy::rollover( lastFileName = newFileName; #endif -#ifdef LOG4CXX_MULTI_PROCESS - return new RolloverDescription( - nextActiveFile, true, renameAction, compressAction); -#else - return new RolloverDescription( - nextActiveFile, false, renameAction, compressAction); -#endif + return new RolloverDescription(nextActiveFile, append, renameAction, compressAction); } - - bool TimeBasedRollingPolicy::isTriggeringEvent( Appender* appender, const log4cxx::spi::LoggingEventPtr& /* event */, http://git-wip-us.apache.org/repos/asf/logging-log4cxx/blob/6854cd00/src/main/include/log4cxx/rolling/fixedwindowrollingpolicy.h ---------------------------------------------------------------------- diff --git a/src/main/include/log4cxx/rolling/fixedwindowrollingpolicy.h b/src/main/include/log4cxx/rolling/fixedwindowrollingpolicy.h index 1cb6b48..aeab8a3 100644 --- a/src/main/include/log4cxx/rolling/fixedwindowrollingpolicy.h +++ b/src/main/include/log4cxx/rolling/fixedwindowrollingpolicy.h @@ -62,8 +62,8 @@ namespace log4cxx { * larger values are specified by the user. * * - * - * + * + * * */ class LOG4CXX_EXPORT FixedWindowRollingPolicy : public RollingPolicyBase { DECLARE_LOG4CXX_OBJECT(FixedWindowRollingPolicy) @@ -100,33 +100,21 @@ namespace log4cxx { void setMaxIndex(int newVal); void setMinIndex(int newVal); - -/** -* Initialize the policy and return any initial actions for rolling file appender. -* -* @param file current value of RollingFileAppender::getFile(). -* @param append current value of RollingFileAppender::getAppend(). -* @param p pool used for any required memory allocations. -* @return Description of the initialization, may be null to indicate -* no initialization needed. -* @throws SecurityException if denied access to log files. -*/ -virtual RolloverDescriptionPtr initialize( -const LogString& file, const bool append, log4cxx::helpers::Pool& p); - -/** -* Prepare for a rollover. This method is called prior to -* closing the active log file, performs any necessary -* preliminary actions and describes actions needed -* after close of current log file. -* -* @param activeFile file name for current active log file. -* @param p pool used for any required memory allocations. -* @return Description of pending rollover, may be null to indicate no rollover -* at this time. -* @throws SecurityException if denied access to log files. -*/ -virtual RolloverDescriptionPtr rollover(const LogString& activeFile, log4cxx::helpers::Pool& p); + /** + * {@inheritDoc} + */ + RolloverDescriptionPtr initialize( + const LogString& currentActiveFile, + const bool append, + log4cxx::helpers::Pool& pool); + + /** + * {@inheritDoc} + */ + RolloverDescriptionPtr rollover( + const LogString& currentActiveFile, + const bool append, + log4cxx::helpers::Pool& pool); protected: log4cxx::pattern::PatternMap getFormatSpecifiers() const; http://git-wip-us.apache.org/repos/asf/logging-log4cxx/blob/6854cd00/src/main/include/log4cxx/rolling/rollingpolicy.h ---------------------------------------------------------------------- diff --git a/src/main/include/log4cxx/rolling/rollingpolicy.h b/src/main/include/log4cxx/rolling/rollingpolicy.h index 4598d94..04a834d 100644 --- a/src/main/include/log4cxx/rolling/rollingpolicy.h +++ b/src/main/include/log4cxx/rolling/rollingpolicy.h @@ -33,8 +33,8 @@ namespace log4cxx { * is also responsible for providing the <em>active log file</em>, * that is the live file where logging output will be directed. * - * - * + * + * * */ class LOG4CXX_EXPORT RollingPolicy : @@ -43,35 +43,39 @@ namespace log4cxx { public: virtual ~RollingPolicy() {} - /** - * Initialize the policy and return any initial actions for rolling file appender. - * - * @param file current value of RollingFileAppender.getFile(). - * @param append current value of RollingFileAppender.getAppend(). - * @param p pool for memory allocations during call. - * @return Description of the initialization, may be null to indicate - * no initialization needed. - * @throws SecurityException if denied access to log files. - */ - virtual RolloverDescriptionPtr initialize( - const LogString& file, - const bool append, - log4cxx::helpers::Pool& p) = 0; - /** - * Prepare for a rollover. This method is called prior to - * closing the active log file, performs any necessary - * preliminary actions and describes actions needed - * after close of current log file. - * - * @param activeFile file name for current active log file. - * @param p pool for memory allocations during call. - * @return Description of pending rollover, may be null to indicate no rollover - * at this time. - * @throws SecurityException if denied access to log files. - */ - virtual RolloverDescriptionPtr rollover(const LogString& activeFile, - log4cxx::helpers::Pool& p) = 0; + /** + * Initialize the policy and return any initial actions for rolling file appender. + * + * @param currentActiveFile current value of RollingFileAppender.getFile(). + * @param append current value of RollingFileAppender.getAppend(). + * @param pool pool for memory allocations during call. + * @return Description of the initialization, may be null to indicate + * no initialization needed. + * @throws SecurityException if denied access to log files. + */ + virtual RolloverDescriptionPtr initialize( + const LogString& currentActiveFile, + const bool append, + log4cxx::helpers::Pool& pool) = 0; + + /** + * Prepare for a rollover. This method is called prior to + * closing the active log file, performs any necessary + * preliminary actions and describes actions needed + * after close of current log file. + * + * @param currentActiveFile file name for current active log file. + * @param append current value of the parent FileAppender.getAppend(). + * @param pool pool for memory allocations during call. + * @return Description of pending rollover, may be null to indicate no rollover + * at this time. + * @throws SecurityException if denied access to log files. + */ + virtual RolloverDescriptionPtr rollover( + const LogString& currentActiveFile, + const bool append, + log4cxx::helpers::Pool& pool) = 0; }; LOG4CXX_PTR_DEF(RollingPolicy); http://git-wip-us.apache.org/repos/asf/logging-log4cxx/blob/6854cd00/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h ---------------------------------------------------------------------- diff --git a/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h b/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h index 284d7a5..4491b0b 100644 --- a/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h +++ b/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h @@ -246,35 +246,21 @@ namespace log4cxx { const std::string createFile(const std::string& filename, const std::string& suffix, log4cxx::helpers::Pool& pool); #endif - /** - * Initialize the policy and return any initial actions for rolling file appender. - * - * @param file current value of RollingFileAppender.getFile(). - * @param append current value of RollingFileAppender.getAppend(). - * @param pool pool for any required allocations. - * @return Description of the initialization, may be null to indicate - * no initialization needed. - * @throws SecurityException if denied access to log files. - */ - RolloverDescriptionPtr initialize( - const LogString& file, - const bool append, - log4cxx::helpers::Pool& pool); - - /** - * Prepare for a rollover. This method is called prior to - * closing the active log file, performs any necessary - * preliminary actions and describes actions needed - * after close of current log file. - * - * @param activeFile file name for current active log file. - * @param pool pool for any required allocations. - * @return Description of pending rollover, may be null to indicate no rollover - * at this time. - * @throws SecurityException if denied access to log files. - */ - RolloverDescriptionPtr rollover(const LogString& activeFile, - log4cxx::helpers::Pool& pool); + /** + * {@inheritDoc} + */ + RolloverDescriptionPtr initialize( + const LogString& currentActiveFile, + const bool append, + log4cxx::helpers::Pool& pool); + + /** + * {@inheritDoc} + */ + RolloverDescriptionPtr rollover( + const LogString& currentActiveFile, + const bool append, + log4cxx::helpers::Pool& pool); /** * Determines if a rollover may be appropriate at this time. If
