Replace deprecated readdir_r with readdir

Change-Id: I83e91612be7ff318d0ee932cc6f4b3ea04a79f12
Signed-off-by: Kyungwook Tak <k.tak@samsung.com>

diff --git a/src/framework/service/file-system.cpp b/src/framework/service/file-system.cpp
index 19e0aaf..2422a4d 100644 (file)
--- a/src/framework/service/file-system.cpp
+++ b/src/framework/service/file-system.cpp
@@ -201,10 +201,51 @@ FilePtr File::createInternal(const std::string &fpath, const FilePtr &parentdir,
                return FilePtr(new File(fpath, parentdir, type, std::move(statptr)));
 }
 
+std::map<std::string, std::mutex> FsVisitor::m_locks;
+std::mutex FsVisitor::m_lockMapCleanMutex;
+
+void FsVisitor::cleanLocks()
+{
+       std::lock_guard<std::mutex> l(FsVisitor::m_lockMapCleanMutex);
+       FsVisitor::m_locks.clear();
+}
+
+FsVisitor::Dir::Dir(const std::string &dirname) : m_dir(nullptr), m_dirname(dirname)
+{
+       if (isInBlackList(this->m_dirname))
+               throw std::invalid_argument("dirname is in blacklist.");
+
+       DEBUG("try lock() on dir: " << this->m_dirname);
+       // get lock ownership before open directory stream
+       FsVisitor::m_locks[this->m_dirname].lock();
+       DEBUG("get lock for dir: " << this->m_dirname);
+
+       if ((this->m_dir = ::opendir(this->m_dirname.c_str())) == nullptr) {
+               // release lock before throw exception because dtor won't be called.
+               FsVisitor::m_locks[this->m_dirname].unlock();
+               throw std::invalid_argument("cannot open dir");
+       }
+}
+
+FsVisitor::Dir::~Dir()
+{
+       ::closedir(this->m_dir);
+       FsVisitor::m_locks[this->m_dirname].unlock();
+       DEBUG("unlocked with closedir: " << this->m_dirname);
+}
+
+DIR *FsVisitor::Dir::get()
+{
+       return this->m_dir;
+}
+
 FsVisitor::DirPtr FsVisitor::openDir(const std::string &dir)
 {
-       return std::unique_ptr<DIR, int(*)(DIR *)>(
-               (isInBlackList(dir) ? nullptr : ::opendir(dir.c_str())), ::closedir);
+       try {
+               return DirPtr(new FsVisitor::Dir(dir));
+       } catch (...) {
+               return DirPtr(nullptr);
+       }
 }
 
 FsVisitorPtr FsVisitor::create(TargetHandler &&targetHandler,
@@ -225,30 +266,28 @@ FsVisitorPtr FsVisitor::create(TargetHandler &&targetHandler,
 FsVisitor::FsVisitor(TargetHandler &&targetHandler, const std::string &dirpath,
                                         bool isBasedOnName, time_t modifiedSince) :
        m_targetHandler(std::move(targetHandler)), m_path(dirpath),
-       m_since(modifiedSince), m_isDone(true), m_isBasedOnName(isBasedOnName),
-       m_entryBuf(static_cast<struct dirent *>(::malloc(offsetof(struct dirent, d_name) + NAME_MAX + 1)))
+       m_since(modifiedSince), m_isDone(true), m_isBasedOnName(isBasedOnName)
 {
-       if (this->m_entryBuf == nullptr)
-               throw std::bad_alloc();
 }
 
 FsVisitor::~FsVisitor()
 {
-       ::free(this->m_entryBuf);
 }
 
 void FsVisitor::run(const DirPtr &dirptr, const FilePtr &currentdir)
 {
-       struct dirent *result = nullptr;
        while (true) {
-               auto ret = ::readdir_r(dirptr.get(), this->m_entryBuf, &result);
-               if (ret != 0) {
-                       WARN("readdir_r error on dir: " << currentdir->getPath() <<
-                                " with errno: " << errno << ". Silently ignore this error & dir stream"
-                                " to reduce side-effect of traversing all the other file systems.");
-                       break;
-               } else if (result == nullptr) {
-                       DEBUG("End of stream of dir: " << currentdir->getPath());
+               errno = 0;
+               auto result = ::readdir(dirptr->get());
+               if (result == nullptr) {
+                       if (errno != 0)
+                               WARN("readdir error on dir: " << currentdir->getPath() <<
+                                        " with errno: " << errno << ". Silently ignore this error &"
+                                        " dir stream to reduce side-effect of traversing all the other"
+                                        " file systems.");
+                       else
+                               DEBUG("end of stream: " << currentdir->getPath());
+
                        break;
                }
 
@@ -267,12 +306,6 @@ void FsVisitor::run(const DirPtr &dirptr, const FilePtr &currentdir)
                                (name_size == 2 && name[0] == '.' && name[1] == '.'))
                                continue;
 
-                       auto ndirptr = openDir(fullpath);
-                       if (ndirptr == nullptr) {
-                               WARN("Failed to open dir: " << fullpath << " with errno: " << errno);
-                               continue;
-                       }
-
                        FilePtr ncurrentdir;
                        try {
                                ncurrentdir = File::create(fullpath, currentdir);
@@ -288,6 +321,12 @@ void FsVisitor::run(const DirPtr &dirptr, const FilePtr &currentdir)
                        if (this->m_isBasedOnName && ncurrentdir->isInApp()) {
                                this->m_targetHandler(ncurrentdir);
                        } else {
+                               auto ndirptr = openDir(fullpath);
+                               if (ndirptr == nullptr) {
+                                       WARN("Failed to open dir: " << fullpath << " with errno: " << errno);
+                                       continue;
+                               }
+
                                DEBUG("recurse dir : " << fullpath);
                                this->run(ndirptr, ncurrentdir);
                        }
@@ -312,6 +351,15 @@ void FsVisitor::run(const DirPtr &dirptr, const FilePtr &currentdir)
 
 void FsVisitor::run()
 {
+       auto currentdir = File::create(this->m_path, nullptr);
+
+       INFO("Visiting files start from dir: " << this->m_path);
+
+       if (this->m_isBasedOnName && currentdir->isInApp()) {
+               this->m_targetHandler(currentdir);
+               return;
+       }
+
        auto dirptr = openDir(this->m_path);
 
        if (dirptr == nullptr) {
@@ -327,14 +375,7 @@ void FsVisitor::run()
                }
        }
 
-       auto currentdir = File::create(this->m_path, nullptr);
-
-       INFO("Visiting files start from dir: " << this->m_path);
-
-       if (this->m_isBasedOnName && currentdir->isInApp())
-               this->m_targetHandler(currentdir);
-       else
-               this->run(dirptr, currentdir);
+       this->run(dirptr, currentdir);
 }
 
 } // namespace Csr

diff --git a/src/framework/service/file-system.h b/src/framework/service/file-system.h
index 70f6ca8..ee73279 100644 (file)
--- a/src/framework/service/file-system.h
+++ b/src/framework/service/file-system.h
@@ -25,6 +25,8 @@
 #include <memory>
 #include <queue>
 #include <functional>
+#include <mutex>
+#include <map>
 
 #include <cstddef>
 #include <ctime>
@@ -147,8 +149,25 @@ public:
                                                           const std::string &dirpath, bool isBasedOnName,
                                                           time_t modifiedSince = -1);
 
+       // clean up per directory lock map.
+       // Warning: should be called only no directory is traversed at the time.
+       // this is essential because mutex exists per directory and it can be
+       // stacked a lot because it won't be freed unless process is terminated.
+       static void cleanLocks();
+
 private:
-       using DirPtr = std::unique_ptr<DIR, int(*)(DIR *)>;
+       class Dir {
+       public:
+               Dir(const std::string &dirname);
+               ~Dir();
+               DIR *get();
+
+       private:
+               DIR *m_dir;
+               std::string m_dirname;
+       };
+
+       using DirPtr = std::unique_ptr<Dir>;
 
        void run(const DirPtr &dirptr, const FilePtr &currentdir);
 
@@ -157,12 +176,14 @@ private:
        FsVisitor(TargetHandler &&targetHandler, const std::string &dirpath,
                          bool isBasedOnName, time_t modifiedSince = -1);
 
+       static std::map<std::string, std::mutex> m_locks;
+       static std::mutex m_lockMapCleanMutex;
+
        TargetHandler m_targetHandler;
        std::string m_path;
        time_t m_since;
        bool m_isDone;
        bool m_isBasedOnName;
-       struct dirent *m_entryBuf;
 };
 
 } // namespace Csr

diff --git a/src/framework/service/thread-pool.cpp b/src/framework/service/thread-pool.cpp
index c28ac45..218c8ff 100644 (file)
--- a/src/framework/service/thread-pool.cpp
+++ b/src/framework/service/thread-pool.cpp
@@ -28,6 +28,10 @@
 #include "common/audit/logger.h"
 #include "common/exception.h"
 
+#ifndef INTERNAL_TEST_BUILD
+#include "service/file-system.h"
+#endif
+
 #define __BEGIN_CRITICAL__ { std::unique_lock<std::mutex> lock(this->m_mutex);
 #define __END_CRITICAL__   }
 
@@ -59,11 +63,15 @@ ThreadPool::ThreadPool(size_t threads) : m_stop(false), m_runningWorkersNum(0)
 
                                DEBUG("Start task on thread[" << std::this_thread::get_id() << "]");
 
+                               __BEGIN_CRITICAL__
                                ++this->m_runningWorkersNum;
+                               __END_CRITICAL__
 
                                task();
 
+                               __BEGIN_CRITICAL__
                                --this->m_runningWorkersNum;
+                               __END_CRITICAL__
                        }
                });
        }
@@ -73,7 +81,16 @@ ThreadPool::ThreadPool(size_t threads) : m_stop(false), m_runningWorkersNum(0)
 
 bool ThreadPool::isTaskRunning() const
 {
-       return this->m_runningWorkersNum != 0;
+       std::lock_guard<std::mutex> l(this->m_mutex);
+
+       if (this->m_runningWorkersNum == 0) {
+#ifndef INTERNAL_TEST_BUILD
+               FsVisitor::cleanLocks();
+#endif
+               return false;
+       } else {
+               return true;
+       }
 }
 
 ThreadPool::~ThreadPool()

diff --git a/src/framework/service/thread-pool.h b/src/framework/service/thread-pool.h
index a808ffa..2f9ac80 100644 (file)
--- a/src/framework/service/thread-pool.h
+++ b/src/framework/service/thread-pool.h
@@ -24,7 +24,6 @@
 
 #include <thread>
 #include <mutex>
-#include <atomic>
 #include <condition_variable>
 #include <functional>
 #include <vector>
@@ -51,10 +50,10 @@ public:
 
 private:
        bool m_stop;
-       std::atomic<size_t> m_runningWorkersNum;
+       size_t m_runningWorkersNum;
        std::vector<std::thread> m_workers;
        std::queue<std::function<void()>> m_tasks;
-       std::mutex m_mutex;
+       mutable std::mutex m_mutex;
        std::condition_variable m_cv;
 };
 

diff --git a/test/engine/content-screening/sample-engine.cpp b/test/engine/content-screening/sample-engine.cpp
index 11063ca..34afff7 100644 (file)
--- a/test/engine/content-screening/sample-engine.cpp
+++ b/test/engine/content-screening/sample-engine.cpp
@@ -465,20 +465,21 @@ int csre_cs_scan_app_on_cloud(csre_cs_context_h handle,
                return CSRE_ERROR_NONE;
        }
 
-       struct dirent entry;
-       struct dirent *result;
+       while (true) {
+               auto result = readdir(dirp.get());
+               if (result == nullptr)
+                       break;
 
-       while (readdir_r(dirp.get(), &entry, &result) == 0 && result != nullptr) {
                std::string fullpath(app_root_dir);
-               std::string filename(entry.d_name);
+               std::string filename(result->d_name);
                fullpath += "/";
                fullpath += filename;
 
                int ret = CSRE_ERROR_NONE;
 
-               if (entry.d_type & (DT_REG | DT_LNK))
+               if (result->d_type & (DT_REG | DT_LNK))
                        ret = csre_cs_scan_file(handle, fullpath.c_str(), &detected);
-               else if ((entry.d_type & DT_DIR)
+               else if ((result->d_type & DT_DIR)
                                 && filename.compare("..") != 0
                                 && filename.compare(".") != 0)
                        ret = csre_cs_scan_app_on_cloud(handle, fullpath.c_str(), &detected);

diff --git a/test/thread-pool/CMakeLists.txt b/test/thread-pool/CMakeLists.txt
index 41a2c58..a569f1e 100644 (file)
--- a/test/thread-pool/CMakeLists.txt
+++ b/test/thread-pool/CMakeLists.txt
@@ -16,6 +16,7 @@
 # @author      Kyungwook Tak (k.tak@samsung.com)
 # @brief       build test program of server thread pool
 #
+
 PKG_CHECK_MODULES(${TARGET_CSR_THREADPOOL_TEST}_DEP
        REQUIRED
 )
@@ -36,6 +37,11 @@ INCLUDE_DIRECTORIES(
        ${${TARGET_CSR_THREADPOOL_TEST}_DEP_INCLUDE_DIRS}
 )
 
+SET_SOURCE_FILES_PROPERTIES(${${TARGET_CSR_THREADPOOL_TEST}_SRCS}
+       PROPERTIES
+               COMPILE_FLAGS "-DINTERNAL_TEST_BUILD"
+)
+
 ADD_EXECUTABLE(${TARGET_CSR_THREADPOOL_TEST} ${${TARGET_CSR_THREADPOOL_TEST}_SRCS})
 
 TARGET_LINK_LIBRARIES(${TARGET_CSR_THREADPOOL_TEST}