This is an automated email from the ASF dual-hosted git repository.
pengzheng pushed a commit to branch feature/refactor_bundle_cache
in repository https://gitbox.apache.org/repos/asf/celix.git
The following commit(s) were added to refs/heads/feature/refactor_bundle_cache
by this push:
new 0d3e3d71 Optimize celix_utils_deleteDirectory to eliminate `stat`
calls and test it thoroughly.
0d3e3d71 is described below
commit 0d3e3d71f2c3d9576368ffea4da4db2d20e1aa59
Author: Pepijn Noltes <[email protected]>
AuthorDate: Fri Mar 31 13:08:50 2023 +0200
Optimize celix_utils_deleteDirectory to eliminate `stat` calls and test it
thoroughly.
---
libs/error_injector/CMakeLists.txt | 1 +
libs/error_injector/{stdio => fts}/CMakeLists.txt | 16 +++---
.../include/stdio_ei.h => fts/include/fts_ei.h} | 12 ++---
.../{stdio/src/stdio_ei.cc => fts/src/fts_ei.cc} | 29 ++++++-----
libs/error_injector/stdio/CMakeLists.txt | 1 +
libs/error_injector/stdio/include/stdio_ei.h | 2 +
libs/error_injector/stdio/src/stdio_ei.cc | 10 ++++
libs/utils/gtest/CMakeLists.txt | 2 +-
.../gtest/src/FileUtilsErrorInjectionTestSuite.cc | 58 +++++++++++++++++++++-
libs/utils/gtest/src/FileUtilsTestSuite.cc | 33 ++++++++++++
libs/utils/src/celix_file_utils.c | 55 ++++++++++----------
11 files changed, 162 insertions(+), 57 deletions(-)
diff --git a/libs/error_injector/CMakeLists.txt
b/libs/error_injector/CMakeLists.txt
index 193873cb..1d95086c 100644
--- a/libs/error_injector/CMakeLists.txt
+++ b/libs/error_injector/CMakeLists.txt
@@ -33,6 +33,7 @@ add_subdirectory(celix_threads)
add_subdirectory(eventfd)
add_subdirectory(celix_bundle_ctx)
add_subdirectory(stat)
+add_subdirectory(fts)
celix_subproject(ERROR_INJECTOR_MDNSRESPONDER "Option to enable building the
mdnsresponder error injector" OFF)
if (ERROR_INJECTOR_MDNSRESPONDER)
diff --git a/libs/error_injector/stdio/CMakeLists.txt
b/libs/error_injector/fts/CMakeLists.txt
similarity index 68%
copy from libs/error_injector/stdio/CMakeLists.txt
copy to libs/error_injector/fts/CMakeLists.txt
index 44882882..b94e0f36 100644
--- a/libs/error_injector/stdio/CMakeLists.txt
+++ b/libs/error_injector/fts/CMakeLists.txt
@@ -15,13 +15,13 @@
# specific language governing permissions and limitations
# under the License.
-add_library(stdio_ei STATIC src/stdio_ei.cc)
+add_library(fts_ei STATIC src/fts_ei.cc)
-target_include_directories(stdio_ei PUBLIC ${CMAKE_CURRENT_LIST_DIR}/include)
-target_link_libraries(stdio_ei PUBLIC Celix::error_injector)
-# It plays nicely with address sanitizer this way.
-target_link_options(stdio_ei INTERFACE
- LINKER:--wrap,fopen
- LINKER:--wrap,fwrite
+target_include_directories(fts_ei PUBLIC ${CMAKE_CURRENT_LIST_DIR}/include)
+target_link_libraries(fts_ei PUBLIC Celix::error_injector)
+
+target_link_options(fts_ei INTERFACE
+ LINKER:--wrap,fts_open
+ LINKER:--wrap,fts_read
)
-add_library(Celix::stdio_ei ALIAS stdio_ei)
+add_library(Celix::fts_ei ALIAS fts_ei)
diff --git a/libs/error_injector/stdio/include/stdio_ei.h
b/libs/error_injector/fts/include/fts_ei.h
similarity index 84%
copy from libs/error_injector/stdio/include/stdio_ei.h
copy to libs/error_injector/fts/include/fts_ei.h
index f528e40d..5bb7862d 100644
--- a/libs/error_injector/stdio/include/stdio_ei.h
+++ b/libs/error_injector/fts/include/fts_ei.h
@@ -17,20 +17,20 @@
under the License.
*/
-#ifndef CELIX_STDIO_EI_H
-#define CELIX_STDIO_EI_H
+#ifndef CELIX_FTS_EI_H
+#define CELIX_FTS_EI_H
#ifdef __cplusplus
extern "C" {
#endif
#include "celix_error_injector.h"
-#include <stdio.h>
+#include <fts.h>
-CELIX_EI_DECLARE(fopen, FILE *);
+CELIX_EI_DECLARE(fts_open, FTS*);
-CELIX_EI_DECLARE(fwrite, size_t);
+CELIX_EI_DECLARE(fts_read, FTSENT*);
#ifdef __cplusplus
}
#endif
-#endif //CELIX_STDIO_EI_H
+#endif //CELIX_FTS_EI_H
diff --git a/libs/error_injector/stdio/src/stdio_ei.cc
b/libs/error_injector/fts/src/fts_ei.cc
similarity index 58%
copy from libs/error_injector/stdio/src/stdio_ei.cc
copy to libs/error_injector/fts/src/fts_ei.cc
index 8165c5fd..9d86fc2c 100644
--- a/libs/error_injector/stdio/src/stdio_ei.cc
+++ b/libs/error_injector/fts/src/fts_ei.cc
@@ -17,26 +17,29 @@
under the License.
*/
+#include "fts_ei.h"
#include <errno.h>
-#include "stdio_ei.h"
extern "C" {
-FILE *__real_fopen (const char *__filename, const char *__modes);
-CELIX_EI_DEFINE(fopen, FILE *)
-FILE *__wrap_fopen (const char *__filename, const char *__modes) {
- errno = EMFILE;
- CELIX_EI_IMPL(fopen);
+
+
+FTS *__real_fts_open (char * const *, int, int (*)(const FTSENT **, const
FTSENT **));
+CELIX_EI_DEFINE(fts_open, FTS*)
+FTS *__wrap_fts_open (char * const *path_argv, int options, int
(*compar)(const FTSENT **, const FTSENT **)) {
+ errno = ENOMEM;
+ CELIX_EI_IMPL(fts_open);
errno = 0;
- return __real_fopen(__filename, __modes);
+ return __real_fts_open(path_argv, options, compar);
}
-size_t __real_fwrite (const void *__restrict __ptr, size_t __size, size_t __n,
FILE *__restrict __s);
-CELIX_EI_DEFINE(fwrite, size_t)
-size_t __wrap_fwrite (const void *__restrict __ptr, size_t __size, size_t __n,
FILE *__restrict __s) {
- errno = ENOSPC;
- CELIX_EI_IMPL(fwrite);
+FTSENT *__real_fts_read (FTS *);
+CELIX_EI_DEFINE(fts_read, FTSENT*)
+FTSENT *__wrap_fts_read (FTS *ftsp) {
+ errno = ENOMEM;
+ CELIX_EI_IMPL(fts_read);
errno = 0;
- return __real_fwrite(__ptr, __size, __n, __s);
+ return __real_fts_read(ftsp);
}
+
}
\ No newline at end of file
diff --git a/libs/error_injector/stdio/CMakeLists.txt
b/libs/error_injector/stdio/CMakeLists.txt
index 44882882..cfae3d68 100644
--- a/libs/error_injector/stdio/CMakeLists.txt
+++ b/libs/error_injector/stdio/CMakeLists.txt
@@ -23,5 +23,6 @@ target_link_libraries(stdio_ei PUBLIC Celix::error_injector)
target_link_options(stdio_ei INTERFACE
LINKER:--wrap,fopen
LINKER:--wrap,fwrite
+ LINKER:--wrap,remove
)
add_library(Celix::stdio_ei ALIAS stdio_ei)
diff --git a/libs/error_injector/stdio/include/stdio_ei.h
b/libs/error_injector/stdio/include/stdio_ei.h
index f528e40d..15c2e0ee 100644
--- a/libs/error_injector/stdio/include/stdio_ei.h
+++ b/libs/error_injector/stdio/include/stdio_ei.h
@@ -30,6 +30,8 @@ CELIX_EI_DECLARE(fopen, FILE *);
CELIX_EI_DECLARE(fwrite, size_t);
+CELIX_EI_DECLARE(remove, int);
+
#ifdef __cplusplus
}
#endif
diff --git a/libs/error_injector/stdio/src/stdio_ei.cc
b/libs/error_injector/stdio/src/stdio_ei.cc
index 8165c5fd..e621b249 100644
--- a/libs/error_injector/stdio/src/stdio_ei.cc
+++ b/libs/error_injector/stdio/src/stdio_ei.cc
@@ -39,4 +39,14 @@ size_t __wrap_fwrite (const void *__restrict __ptr, size_t
__size, size_t __n, F
errno = 0;
return __real_fwrite(__ptr, __size, __n, __s);
}
+
+
+int __real_remove (const char *__filename);
+CELIX_EI_DEFINE(remove, int)
+int __wrap_remove (const char *__filename) {
+ errno = EACCES;
+ CELIX_EI_IMPL(remove);
+ errno = 0;
+ return __real_remove(__filename);
+}
}
\ No newline at end of file
diff --git a/libs/utils/gtest/CMakeLists.txt b/libs/utils/gtest/CMakeLists.txt
index 2527d6a0..27482733 100644
--- a/libs/utils/gtest/CMakeLists.txt
+++ b/libs/utils/gtest/CMakeLists.txt
@@ -80,7 +80,7 @@ if (LINKER_WRAP_SUPPORTED)
add_executable(test_utils_with_ei
src/FileUtilsErrorInjectionTestSuite.cc
)
- target_link_libraries(test_utils_with_ei PRIVATE Celix::zip_ei
Celix::stdio_ei Celix::stat_ei Celix::utils_obj Celix::utils_ei GTest::gtest
GTest::gtest_main)
+ target_link_libraries(test_utils_with_ei PRIVATE Celix::zip_ei
Celix::stdio_ei Celix::stat_ei Celix::fts_ei Celix::utils_obj Celix::utils_ei
GTest::gtest GTest::gtest_main)
target_include_directories(test_utils_with_ei PRIVATE ../src) #for
version_private (needs refactoring of test)
celix_deprecated_utils_headers(test_utils_with_ei)
add_dependencies(test_utils_with_ei test_utils_resources)
diff --git a/libs/utils/gtest/src/FileUtilsErrorInjectionTestSuite.cc
b/libs/utils/gtest/src/FileUtilsErrorInjectionTestSuite.cc
index 139b95a6..544f371b 100644
--- a/libs/utils/gtest/src/FileUtilsErrorInjectionTestSuite.cc
+++ b/libs/utils/gtest/src/FileUtilsErrorInjectionTestSuite.cc
@@ -20,6 +20,7 @@
#include <fstream>
#include <gtest/gtest.h>
#include <stdlib.h>
+#include <string.h>
#include <string>
#include <thread>
#include <unistd.h>
@@ -28,6 +29,7 @@
#include "celix_file_utils.h"
#include "celix_properties.h"
#include "celix_utils_ei.h"
+#include "fts_ei.h"
#include "stat_ei.h"
#include "stdio_ei.h"
#include "zip_ei.h"
@@ -42,8 +44,11 @@ public:
celix_ei_expect_celix_utils_strdup(nullptr, 0, nullptr);
celix_ei_expect_fopen(nullptr, 0, nullptr);
celix_ei_expect_fwrite(nullptr, 0, 0);
+ celix_ei_expect_remove(nullptr, 0, 0);
celix_ei_expect_mkdir(nullptr, 0, 0);
celix_ei_expect_stat(nullptr, 0, 0);
+ celix_ei_expect_fts_open(nullptr, 0, nullptr);
+ celix_ei_expect_fts_read(nullptr, 0, nullptr);
}
};
@@ -147,4 +152,55 @@ TEST_F(FileUtilsWithErrorInjectionTestSuite,
CreateDirectory) {
status = celix_utils_createDirectory(testDir.c_str(), true, &error);
EXPECT_EQ(status, CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,EOVERFLOW));
EXPECT_NE(error, nullptr);
-}
\ No newline at end of file
+ celix_utils_deleteDirectory(root.c_str(), nullptr);
+}
+
+TEST_F(FileUtilsWithErrorInjectionTestSuite, DeleteDirectory) {
+ const std::string root = "celix_file_utils_test";
+ const std::string testDir = root + "/directory";
+
+ auto status = celix_utils_createDirectory(root.c_str(), false, nullptr);
+ EXPECT_EQ(status, CELIX_SUCCESS);
+ celix_ei_expect_fts_open(CELIX_EI_UNKNOWN_CALLER, 0, nullptr);
+ celix_ei_expect_fts_read(nullptr, 0, nullptr);
+ const char* error = nullptr;
+ status = celix_utils_deleteDirectory(root.c_str(), &error);
+ EXPECT_EQ(status, CELIX_ENOMEM);
+ EXPECT_NE(error, nullptr);
+ EXPECT_TRUE(celix_utils_directoryExists(root.c_str()));
+
+ status = celix_utils_createDirectory(testDir.c_str(), false, nullptr);
+ EXPECT_EQ(status, CELIX_SUCCESS);
+ celix_ei_expect_fts_open(nullptr, 0, nullptr);
+ // fts_open followed by fts_close without calling fts_read will cause
memory leak reported by ASAN
+ celix_ei_expect_fts_read(CELIX_EI_UNKNOWN_CALLER, 0, nullptr, 2);
+ status = celix_utils_deleteDirectory(root.c_str(), &error);
+ EXPECT_EQ(status, CELIX_ENOMEM);
+ EXPECT_NE(error, nullptr);
+ EXPECT_TRUE(celix_utils_directoryExists(root.c_str()));
+
+ status = celix_utils_createDirectory(testDir.c_str(), false, nullptr);
+ EXPECT_EQ(status, CELIX_SUCCESS);
+ FTSENT ent;
+ memset(&ent, 0, sizeof(ent));
+ ent.fts_info = FTS_DNR;
+ ent.fts_errno = EACCES;
+ celix_ei_expect_fts_open(nullptr, 0, nullptr);
+ // fts_open followed by fts_close without calling fts_read will cause
memory leak reported by ASAN
+ celix_ei_expect_fts_read(CELIX_EI_UNKNOWN_CALLER, 0, &ent, 2);
+ status = celix_utils_deleteDirectory(root.c_str(), &error);
+ EXPECT_EQ(status, CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,EACCES));
+ EXPECT_NE(error, nullptr);
+ EXPECT_TRUE(celix_utils_directoryExists(root.c_str()));
+
+ status = celix_utils_createDirectory(testDir.c_str(), false, nullptr);
+ EXPECT_EQ(status, CELIX_SUCCESS);
+ celix_ei_expect_remove(CELIX_EI_UNKNOWN_CALLER, 0, -1);
+ status = celix_utils_deleteDirectory(root.c_str(), &error);
+ EXPECT_EQ(status, CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,EACCES));
+ EXPECT_NE(error, nullptr);
+ EXPECT_TRUE(celix_utils_directoryExists(root.c_str()));
+
+ status = celix_utils_deleteDirectory(root.c_str(), &error);
+ EXPECT_EQ(status, CELIX_SUCCESS);
+}
diff --git a/libs/utils/gtest/src/FileUtilsTestSuite.cc
b/libs/utils/gtest/src/FileUtilsTestSuite.cc
index 79bc80ea..0b4755a9 100644
--- a/libs/utils/gtest/src/FileUtilsTestSuite.cc
+++ b/libs/utils/gtest/src/FileUtilsTestSuite.cc
@@ -17,6 +17,7 @@
* under the License.
*/
+#include <fstream>
#include <gtest/gtest.h>
#include <stdlib.h>
#include <string>
@@ -132,6 +133,38 @@ TEST_F(FileUtilsTestSuite, CreateAndDeleteDirectory) {
EXPECT_EQ(status, CELIX_SUCCESS);
}
+TEST_F(FileUtilsTestSuite, DeleteFileAsDirectory) {
+ const std::string root = "celix_file_utils_test";
+ celix_utils_deleteDirectory(root.c_str(), nullptr);
+ const char* error = nullptr;
+ celix_utils_createDirectory(root.c_str(), true, nullptr);
+ const std::string filename = root + "/file";
+ std::fstream file(filename, std::ios::out);
+ file.close();
+ auto status = celix_utils_deleteDirectory(filename.c_str(), &error);
+ EXPECT_EQ(status, CELIX_FILE_IO_EXCEPTION);
+ EXPECT_NE(error, nullptr);
+ EXPECT_TRUE(celix_utils_fileExists(filename.c_str()));
+ celix_utils_deleteDirectory(root.c_str(), nullptr);
+}
+
+TEST_F(FileUtilsTestSuite, DeleteSymbolicLinkToDirectory) {
+ const std::string root = "celix_file_utils_test";
+ const std::string testDir = root + "/directory";
+ const std::string symLink = root + "/link";
+ celix_utils_deleteDirectory(root.c_str(), nullptr);
+ const char* error = nullptr;
+ celix_utils_createDirectory(testDir.c_str(), true, nullptr);
+ auto status = symlink("./directory", symLink.c_str()); // link ->
./directory
+ EXPECT_EQ(status, 0);
+ status = celix_utils_deleteDirectory(symLink.c_str(), &error);
+ EXPECT_EQ(status, CELIX_SUCCESS);
+ EXPECT_EQ(error, nullptr);
+ EXPECT_FALSE(celix_utils_fileExists(symLink.c_str()));
+ EXPECT_TRUE(celix_utils_directoryExists(testDir.c_str()));
+ celix_utils_deleteDirectory(root.c_str(), nullptr);
+}
+
TEST_F(FileUtilsTestSuite, ExtractZipFileTest) {
std::cout << "Using test zip location " << TEST_ZIP_LOCATION << std::endl;
const char* extractLocation = "extract_location";
diff --git a/libs/utils/src/celix_file_utils.c
b/libs/utils/src/celix_file_utils.c
index 66abddd4..dcfd6b70 100644
--- a/libs/utils/src/celix_file_utils.c
+++ b/libs/utils/src/celix_file_utils.c
@@ -172,37 +172,36 @@ celix_status_t celix_utils_deleteDirectory(const char*
path, const char** errorO
//file exist and is directory
celix_status_t status = CELIX_SUCCESS;
char *paths[] = { (char*)path, NULL };
- FTS *fts = fts_open(paths, FTS_PHYSICAL | FTS_XDEV | FTS_NOCHDIR, NULL);
+ FTS *fts = fts_open(paths, FTS_PHYSICAL | FTS_XDEV | FTS_NOCHDIR |
FTS_NOSTAT, NULL);
if (fts == NULL) {
- status = CELIX_FILE_IO_EXCEPTION;
- *errorOut = strerror(errno);
- } else {
- FTSENT *ent;
- while ((ent = fts_read(fts)) != NULL) {
- switch (ent->fts_info) {
- case FTS_DP:
- case FTS_F:
- case FTS_SL:
- case FTS_SLNONE:
- if (remove(ent->fts_accpath) != 0) {
- status = CELIX_FILE_IO_EXCEPTION;
- *errorOut = strerror(errno);
- }
- break;
- case FTS_DNR:
- case FTS_ERR:
- case FTS_NS:
- status = CELIX_FILE_IO_EXCEPTION;
- *errorOut = strerror(ent->fts_errno);
- break;
- default:
- break;
- }
- if (status != CELIX_SUCCESS) {
+ goto out;
+ }
+ FTSENT *ent = NULL;
+ while ((ent = fts_read(fts)) != NULL) {
+ switch (ent->fts_info) {
+ case FTS_DP:
+ case FTS_NSOK:
+ case FTS_SL:
+ case FTS_SLNONE:
+ if (remove(ent->fts_accpath) != 0) {
+ goto out;
+ }
+ break;
+ case FTS_DNR:
+ case FTS_ERR:
+ errno = ent->fts_errno;
+ goto out;
+ default:
break;
- }
}
- fts_close(fts);
+ }
+out:
+ if (errno != 0) {
+ status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
+ *errorOut = strerror(errno);
+ }
+ if (fts != NULL) {
+ fts_close(fts); // it may change errno
}
return status;
}
