diff --git a/doc/changelog.qbk b/doc/changelog.qbk index ca16ea2..ab5f8b4 100644 --- a/doc/changelog.qbk +++ b/doc/changelog.qbk @@ -11,6 +11,13 @@ [heading 2.24, Boost 1.79] +[*General changes:] + +* In [link log.detailed.sink_backends.text_file `text_file_backend`], added support for appending to a previously written log file, when file rotation is used and log file names use file counters. For this to work, the target storage directory must be the same as the directory where the active log file is written, and `text_file_backend::scan_for_files` must be called prior to emitting any log records to discover the last file counter used in the target storage directory. Previously, in such a configuration the sink backend would generate a new file name instead of appending to the last written one. +* *Breaking change:* The [class_file_collector] interface has changed: + * `scan_for_files` method returns a `scan_result` structure that contains information collected during the scan; + * `is_in_storage` method added for testing if a path refers to a file within the target storage directory. + [*Bug fixes:] * Fixed file counter being set to zero if the user calls [link log.detailed.sink_backends.text_file.file_scanning `text_file_backend::scan_for_files`] multiple times, and the second and the following calls don't find any new files. ([github_issue 179]) diff --git a/doc/sink_backends.qbk b/doc/sink_backends.qbk index e30505c..0894fb0 100644 --- a/doc/sink_backends.qbk +++ b/doc/sink_backends.qbk @@ -210,7 +210,7 @@ The sink backend supports appending to the previously written files (e.g. left f When initializing from [link log.detailed.utilities.setup.settings settings], the "Append" parameter of the "TextFile" sink enables appending. -In order for file appending to actually happen, it is important that the name of the newly opened log file matches the previously written file. Othewise, the sink will simply create a new file under the new name. There are several recommendations to follow when file appending is desirable: +In order for file appending to actually happen, it is important that the name of the newly opened log file matches the previously written file. Otherwise, the sink will simply create a new file under the new name. There are several recommendations to follow when file appending is desirable: * Don't use placeholders in the active file name pattern. This will ensure that every time the sink opens a file for writing, that file has the same name. * Use a distinct target file name pattern, preferably with date, time or counter placeholders. This will ensure that when the file is rotated and collected, it doesn't clash with the previously written files, and that the newly opened file will have a different name from the previous one. diff --git a/include/boost/log/sinks/text_file_backend.hpp b/include/boost/log/sinks/text_file_backend.hpp index 719acc9..48ce8a7 100644 --- a/include/boost/log/sinks/text_file_backend.hpp +++ b/include/boost/log/sinks/text_file_backend.hpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -66,6 +67,20 @@ enum scan_method scan_all //!< Scan for all files in the directory }; +//! The structure contains filesystem scanning results +struct scan_result +{ + //! The number of found files + uintmax_t found_count; + //! If populated, the largest file counter that was used in the found file names + boost::optional< unsigned int > last_file_counter; + + scan_result() BOOST_NOEXCEPT : + found_count(0u) + { + } +}; + /*! * \brief Base class for file collectors * @@ -81,7 +96,7 @@ struct BOOST_LOG_NO_VTABLE collector /*! * Virtual destructor */ - virtual ~collector() {} + BOOST_DEFAULTED_FUNCTION(virtual ~collector(), {}) /*! * The function stores the specified file in the storage. May lead to an older file @@ -91,6 +106,13 @@ struct BOOST_LOG_NO_VTABLE collector */ virtual void store_file(filesystem::path const& src_path) = 0; + /*! + * The function checks if the specified path refers to an existing file in the storage. + * + * \param src_path The path to be checked + */ + virtual bool is_in_storage(filesystem::path const& src_path) const = 0; + /*! * Scans the target directory for the files that have already been stored. The found * files are added to the collector in order to be tracked and erased, if needed. @@ -110,19 +132,17 @@ struct BOOST_LOG_NO_VTABLE collector * \param method The method of scanning. If \c no_scan is specified, the call has no effect. * \param pattern The file name pattern if \a method is \c scan_matching. Otherwise the parameter * is not used. - * \param counter If not \c NULL and \a method is \c scan_matching, the method suggests initial value - * of a file counter that may be used in the file name pattern. The parameter - * is not used otherwise. - * \return The number of found files. + * \return The result of filesystem scanning. The last file counter is only populated if + * \a method is \c scan_matching, the \a pattern contains %N placeholder, and at least + * one file matching the pattern is found. * * \note In case if \a method is \c scan_matching the effect of this function is highly dependent * on the \a pattern definition. It is recommended to choose patterns with easily * distinguished placeholders (i.e. having delimiters between them). Otherwise * either some files can be mistakenly found or not found, which in turn may lead - * to an incorrect file deletion. + * to deletion of an unintended file. */ - virtual uintmax_t scan_for_files( - scan_method method, filesystem::path const& pattern = filesystem::path(), unsigned int* counter = 0) = 0; + virtual scan_result scan_for_files(scan_method method, filesystem::path const& pattern = filesystem::path()) = 0; BOOST_DELETED_FUNCTION(collector(collector const&)) BOOST_DELETED_FUNCTION(collector& operator= (collector const&)) diff --git a/src/text_file_backend.cpp b/src/text_file_backend.cpp index 18f3bb2..8b8920e 100644 --- a/src/text_file_backend.cpp +++ b/src/text_file_backend.cpp @@ -32,10 +32,10 @@ #include #include #include +#include #include #include #include -#include #include #include #include @@ -79,9 +79,7 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { typedef filesystem::filesystem_error filesystem_error; //! A possible Boost.Filesystem extension - renames or moves the file to the target storage - inline void move_file( - filesystem::path const& from, - filesystem::path const& to) + inline void move_file(filesystem::path const& from, filesystem::path const& to) { #if defined(BOOST_WINDOWS_API) // On Windows MoveFile already does what we need @@ -610,6 +608,38 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { //! Information about a single stored file struct file_info { + //! Ordering predicate by timestamp + struct order_by_timestamp + { + typedef bool result_type; + + result_type operator()(file_info const& left, file_info const& right) const BOOST_NOEXCEPT + { + return left.m_TimeStamp < right.m_TimeStamp; + } + }; + + //! Predicate for testing if a file_info refers to a file equivalent to another path + class equivalent_file + { + public: + typedef bool result_type; + + private: + filesystem::path const& m_Path; + + public: + explicit equivalent_file(filesystem::path const& path) BOOST_NOEXCEPT : + m_Path(path) + { + } + + result_type operator()(file_info const& info) const + { + return filesystem::equivalent(info.m_Path, m_Path); + } + }; + uintmax_t m_Size; std::time_t m_TimeStamp; filesystem::path m_Path; @@ -664,9 +694,11 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { //! The function stores the specified file in the storage void store_file(filesystem::path const& file_name) BOOST_OVERRIDE; + //! The function checks if the specified path refers to an existing file in the storage + bool is_in_storage(filesystem::path const& src_path) const BOOST_OVERRIDE; + //! Scans the target directory for the files that have already been stored - uintmax_t scan_for_files( - file::scan_method method, filesystem::path const& pattern, unsigned int* counter) BOOST_OVERRIDE; + file::scan_result scan_for_files(file::scan_method method, filesystem::path const& pattern) BOOST_OVERRIDE; //! The function updates storage restrictions void update(uintmax_t max_size, uintmax_t min_free_space, uintmax_t max_files); @@ -679,7 +711,7 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { private: //! Makes relative path absolute with respect to the base path - filesystem::path make_absolute(filesystem::path const& p) + filesystem::path make_absolute(filesystem::path const& p) const { return filesystem::absolute(p, m_BasePath); } @@ -779,8 +811,8 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { // Check if the file is already in the target directory filesystem::path src_dir = src_path.has_parent_path() ? - filesystem::system_complete(src_path.parent_path()) : - m_BasePath; + filesystem::system_complete(src_path.parent_path()) : + m_BasePath; const bool is_in_target_dir = filesystem::equivalent(src_dir, m_StorageDir); if (!is_in_target_dir) { @@ -888,11 +920,34 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { m_TotalSize += info.m_Size; } - //! Scans the target directory for the files that have already been stored - uintmax_t file_collector::scan_for_files( - file::scan_method method, filesystem::path const& pattern, unsigned int* counter) + //! The function checks if the specified path refers to an existing file in the storage + bool file_collector::is_in_storage(filesystem::path const& src_path) const { - uintmax_t file_count = 0u; + const filesystem::path file_name_path = src_path.filename(); + const filesystem::path trg_path = m_StorageDir / file_name_path; + + // Check if the file is already in the target directory + system::error_code ec; + filesystem::path src_dir = src_path.has_parent_path() ? + filesystem::system_complete(src_path.parent_path(), ec) : + m_BasePath; + if (ec) + return false; + + filesystem::file_status status = filesystem::status(trg_path, ec); + if (ec || status.type() != filesystem::regular_file) + return false; + bool equiv = filesystem::equivalent(src_dir / file_name_path, trg_path, ec); + if (ec) + return false; + + return equiv; + } + + //! Scans the target directory for the files that have already been stored + file::scan_result file_collector::scan_for_files(file::scan_method method, filesystem::path const& pattern) + { + file::scan_result result; if (method != file::no_scan) { filesystem::path dir = m_StorageDir; @@ -903,20 +958,11 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { if (pattern.has_parent_path()) dir = make_absolute(pattern.parent_path()); } - else - { - counter = NULL; - } system::error_code ec; filesystem::file_status status = filesystem::status(dir, ec); if (status.type() == filesystem::directory_file) { - unsigned int new_counter = 0u; - if (counter) - new_counter = *counter; - bool found_file_number = false; - BOOST_LOG_EXPR_IF_MT(lock_guard< mutex > lock(m_Mutex);) file_list files; @@ -931,18 +977,10 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { if (status.type() == filesystem::regular_file) { // Check that there are no duplicates in the resulting list - struct local - { - static bool equivalent(filesystem::path const& left, file_info const& right) - { - return filesystem::equivalent(left, right.m_Path); - } - }; - if (std::find_if(m_Files.begin(), m_Files.end(), - boost::bind(&local::equivalent, boost::cref(info.m_Path), boost::placeholders::_1)) == m_Files.end()) + if (std::find_if(m_Files.begin(), m_Files.end(), file_info::equivalent_file(info.m_Path)) == m_Files.end()) { // Check that the file name matches the pattern - unsigned int file_number = 0; + unsigned int file_number = 0u; bool file_number_parsed = false; if (method != file::scan_matching || match_pattern(filename_string(info.m_Path), mask, file_number, file_number_parsed)) @@ -951,14 +989,11 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { total_size += info.m_Size; info.m_TimeStamp = filesystem::last_write_time(info.m_Path); files.push_back(info); - ++file_count; + ++result.found_count; - // Test that the file_number >= new_counter accounting for the integer overflow - if (file_number_parsed && (file_number - new_counter) < ((~0u) ^ ((~0u) >> 1u))) - { - found_file_number = true; - new_counter = file_number + 1u; - } + // Test that the file_number >= result.last_file_counter accounting for the integer overflow + if (file_number_parsed && (!result.last_file_counter || (file_number - *result.last_file_counter) < ((~0u) ^ ((~0u) >> 1u)))) + result.last_file_counter = file_number; } } } @@ -967,14 +1002,11 @@ BOOST_LOG_ANONYMOUS_NAMESPACE { // Sort files chronologically m_Files.splice(m_Files.end(), files); m_TotalSize += total_size; - m_Files.sort(boost::bind(&file_info::m_TimeStamp, boost::placeholders::_1) < boost::bind(&file_info::m_TimeStamp, boost::placeholders::_2)); - - if (counter && found_file_number) - *counter = new_counter; + m_Files.sort(file_info::order_by_timestamp()); } } - return file_count; + return result; } //! The function updates storage restrictions @@ -1229,7 +1261,7 @@ struct text_file_backend::implementation //! Target file name generator (according to m_TargetFileNamePattern) boost::log::aux::light_function< path_string_type (unsigned int) > m_TargetFileNameGenerator; - //! Stored files counter + //! Counter to use in file names unsigned int m_FileCounter; //! File open mode @@ -1260,6 +1292,11 @@ struct text_file_backend::implementation //! The flag indicates whether the final rotation should be performed bool m_FinalRotationEnabled; + //! The flag indicates that \c m_FileCounter is set to the last used counter value + bool m_FileCounterIsLastUsed; + //! The flag indicates whether the next opened file will be the first file opened by this backend + bool m_IsFirstFile; + implementation(uintmax_t rotation_size, auto_newline_mode auto_newline, bool auto_flush, bool enable_final_rotation) : m_FileCounter(0u), m_FileOpenMode(std::ios_base::trunc | std::ios_base::out), @@ -1267,7 +1304,9 @@ struct text_file_backend::implementation m_FileRotationSize(rotation_size), m_AutoNewlineMode(auto_newline), m_AutoFlush(auto_flush), - m_FinalRotationEnabled(enable_final_rotation) + m_FinalRotationEnabled(enable_final_rotation), + m_FileCounterIsLastUsed(false), + m_IsFirstFile(true) { } }; @@ -1383,13 +1422,49 @@ BOOST_LOG_API void text_file_backend::consume(record_view const& rec, string_typ rotate_file(); } - if (!m_pImpl->m_File.is_open()) + while (!m_pImpl->m_File.is_open()) { filesystem::path new_file_name; if (!use_prev_file_name) - new_file_name = m_pImpl->m_StorageDir / m_pImpl->m_FileNameGenerator(m_pImpl->m_FileCounter++); + { + unsigned int file_counter = m_pImpl->m_FileCounter; + if (BOOST_LIKELY(m_pImpl->m_FileCounterIsLastUsed)) + { + // If the sink backend is configured to append to a previously written file, don't + // increment the file counter and try to open the existing file. Only do this if the + // file is not moved to a different storage location by the file collector. + bool increment_file_counter = true; + if (BOOST_UNLIKELY(m_pImpl->m_IsFirstFile && (m_pImpl->m_FileOpenMode & std::ios_base::app) != 0)) + { + filesystem::path last_file_name = m_pImpl->m_StorageDir / m_pImpl->m_FileNameGenerator(file_counter); + if (!!m_pImpl->m_pFileCollector) + { + increment_file_counter = !m_pImpl->m_pFileCollector->is_in_storage(last_file_name); + } + else + { + system::error_code ec; + increment_file_counter = filesystem::status(last_file_name, ec).type() != filesystem::regular_file; + } + } + + if (BOOST_LIKELY(increment_file_counter)) + { + ++file_counter; + m_pImpl->m_FileCounter = file_counter; + } + } + else + { + m_pImpl->m_FileCounterIsLastUsed = true; + } + + new_file_name = m_pImpl->m_StorageDir / m_pImpl->m_FileNameGenerator(file_counter); + } else + { prev_file_name.swap(new_file_name); + } filesystem::create_directories(new_file_name.parent_path()); @@ -1402,11 +1477,47 @@ BOOST_LOG_API void text_file_backend::consume(record_view const& rec, string_typ system::error_code(system::errc::io_error, system::generic_category()))); } m_pImpl->m_FileName.swap(new_file_name); + m_pImpl->m_IsFirstFile = false; + + // Check the file size before invoking the open handler, as it may write more data to the file + m_pImpl->m_CharactersWritten = static_cast< std::streamoff >(m_pImpl->m_File.tellp()); + if (m_pImpl->m_CharactersWritten + formatted_message.size() >= m_pImpl->m_FileRotationSize) + { + // Avoid running the close handler, as we haven't run the open handler yet + struct close_handler_backup_guard + { + explicit close_handler_backup_guard(close_handler_type& orig_close_handler) BOOST_NOEXCEPT : + m_orig_close_handler(orig_close_handler) + { + orig_close_handler.swap(m_backup_close_handler); + } + ~close_handler_backup_guard() BOOST_NOEXCEPT + { + m_orig_close_handler.swap(m_backup_close_handler); + } + + private: + close_handler_type& m_orig_close_handler; + close_handler_type m_backup_close_handler; + } + close_handler_guard(m_pImpl->m_CloseHandler); + + rotate_file(); + continue; + } if (!m_pImpl->m_OpenHandler.empty()) + { m_pImpl->m_OpenHandler(m_pImpl->m_File); - m_pImpl->m_CharactersWritten = static_cast< std::streamoff >(m_pImpl->m_File.tellp()); + // Update the size of the written data, but don't rotate the file. If the open handler + // exceeds the file size limit we could end up in an infinite loop, as we are constantly + // rotating the file and immediately exceeding its size limit after the open handler is run. + // Write the log record and then rotate the file upon the next log record. + m_pImpl->m_CharactersWritten = static_cast< std::streamoff >(m_pImpl->m_File.tellp()); + } + + break; } m_pImpl->m_File.write(formatted_message.data(), static_cast< std::streamsize >(formatted_message.size())); @@ -1499,8 +1610,7 @@ BOOST_LOG_API void text_file_backend::rotate_file() { if (!!m_pImpl->m_TargetFileNameGenerator) { - // File counter was incremented when the file was opened, we have to use the same counter value we used to generate the original filename - filesystem::path new_file_name = m_pImpl->m_TargetStorageDir / m_pImpl->m_TargetFileNameGenerator(m_pImpl->m_FileCounter - 1u); + filesystem::path new_file_name = m_pImpl->m_TargetStorageDir / m_pImpl->m_TargetFileNameGenerator(m_pImpl->m_FileCounter); if (new_file_name != prev_file_name) { @@ -1555,20 +1665,27 @@ BOOST_LOG_API filesystem::path text_file_backend::get_current_file_name() const //! Performs scanning of the target directory for log files BOOST_LOG_API uintmax_t text_file_backend::scan_for_files(file::scan_method method, bool update_counter) { - if (BOOST_LIKELY(!!m_pImpl->m_pFileCollector)) - { - unsigned int* counter = update_counter ? &m_pImpl->m_FileCounter : static_cast< unsigned int* >(NULL); - return m_pImpl->m_pFileCollector->scan_for_files - ( - method, - m_pImpl->m_TargetFileNamePattern.empty() ? m_pImpl->m_FileNamePattern : m_pImpl->m_TargetFileNamePattern, - counter - ); - } - else + if (BOOST_UNLIKELY(!m_pImpl->m_pFileCollector)) { BOOST_LOG_THROW_DESCR(setup_error, "File collector is not set"); } + + file::scan_result result = m_pImpl->m_pFileCollector->scan_for_files + ( + method, + m_pImpl->m_TargetFileNamePattern.empty() ? m_pImpl->m_FileNamePattern : m_pImpl->m_TargetFileNamePattern + ); + + if (update_counter && !!result.last_file_counter) + { + if (!m_pImpl->m_FileCounterIsLastUsed || (*result.last_file_counter - m_pImpl->m_FileCounter) < ((~0u) ^ ((~0u) >> 1u))) + { + m_pImpl->m_FileCounter = *result.last_file_counter; + m_pImpl->m_FileCounterIsLastUsed = true; + } + } + + return result.found_count; } } // namespace sinks