You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/04/27 07:51:33 UTC
[GitHub] [nifi-minifi-cpp] msharee9 opened a new pull request #772: MINIFICPP-1153 Fix failing CTailFile Tests
msharee9 opened a new pull request #772:
URL: https://github.com/apache/nifi-minifi-cpp/pull/772
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
The intention of this PR was to fix failing CTailfile tests that were added a while ago. The tests were failing because they were accessing same file when run concurrently.
In the process, fixed indentation as well otherwise much of the code shown changed in the PR is not new code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #772: MINIFICPP-1153 Fix failing CTailFile Tests
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #772:
URL: https://github.com/apache/nifi-minifi-cpp/pull/772#discussion_r420255458
##########
File path: nanofi/src/core/file_utils.c
##########
@@ -28,114 +28,222 @@
#include "core/string_utils.h"
#include "core/file_utils.h"
-#ifdef _MSC_VER
+#ifdef WIN32
#ifndef PATH_MAX
#define PATH_MAX 260
#endif
#endif
+#ifdef WIN32
+#define stat _stat
+#define mkdir _mkdir
+#endif
+
int is_directory(const char * path) {
- struct stat dir_stat;
- if (stat(path, &dir_stat) < 0) {
- return 0;
- }
- return S_ISDIR(dir_stat.st_mode);
+ struct stat dir_stat;
+ if (stat(path, &dir_stat) < 0) {
+ return -1;
+ }
+#ifdef WIN32
+ return dir_stat.st_mode & S_IFDIR;
+#else
+ return S_ISDIR(dir_stat.st_mode);
+#endif
}
-const char * get_separator(int force_posix)
-{
+const char * get_separator(int force_posix) {
#ifdef WIN32
- if (!force_posix) {
- return "\\";
- }
+ if (!force_posix) {
+ return "\\";
+ }
#endif
- return "/";
+ return "/";
}
char * concat_path(const char * parent, const char * child) {
- char * path = (char *)malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
- strcpy(path, parent);
- const char * sep = get_separator(0);
- strcat(path, sep);
- strcat(path, child);
- return path;
+ char * path = (char *) malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
+ strcpy(path, parent);
+ const char * sep = get_separator(0);
+ strcat(path, sep);
+ strcat(path, child);
+ return path;
}
+#ifndef WIN32
void remove_directory(const char * dir_path) {
-
- if (!is_directory(dir_path)) {
- if (unlink(dir_path) == -1) {
- printf("Could not remove file %s\n", dir_path);
- }
- return;
+ if (!is_directory(dir_path)) {
+ if (unlink(dir_path) == -1) {
+ printf("Could not remove file %s\n", dir_path);
}
+ return;
+ }
- uint64_t path_len = strlen(dir_path);
- struct dirent * dir;
- DIR * d = opendir(dir_path);
-
- while ((dir = readdir(d)) != NULL) {
- char * entry_name = dir->d_name;
- if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
- continue;
- }
- char * path = concat_path(dir_path, entry_name);
- remove_directory(path);
- free(path);
+ uint64_t path_len = strlen(dir_path);
+ struct dirent * dir;
+ DIR * d = opendir(dir_path);
+
+ while ((dir = readdir(d)) != NULL) {
+ char * entry_name = dir->d_name;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
+ continue;
}
+ char * path = concat_path(dir_path, entry_name);
+ remove_directory(path);
+ free(path);
+ }
+ rmdir(dir_path);
+
+ closedir(d);
+}
+#else
+void remove_directory(const char * dir_path) {
+ HANDLE hFind;
+ WIN32_FIND_DATA fd;
- rmdir(dir_path);
- closedir(d);
+ hFind = FindFirstFile(dir_path, &fd);
+ if (hFind == INVALID_HANDLE_VALUE) {
+ return;
+ }
+
+ if (fd.dwFileAttributes != FILE_ATTRIBUTE_DIRECTORY) {
+ DeleteFile(dir_path);
+ FindClose(hFind);
+ return;
+ }
+
+ char * path = concat_path(dir_path, "*");
+ HANDLE hFind1;
+ if ((hFind1 = FindFirstFile(path, &fd)) != INVALID_HANDLE_VALUE) {
+ do {
+ char * entry_name = fd.cFileName;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) continue;
+ char * entry_path = concat_path(dir_path, entry_name);
+ remove_directory(entry_path);
+ free(entry_path);
+ }while (FindNextFile(hFind1, &fd));
+ }
+ RemoveDirectory(dir_path);
+ FindClose(hFind);
+ FindClose(hFind1);
+ free(path);
}
+#endif
int make_dir(const char * path) {
- if (!path) return -1;
+ if (!path)
+ return -1;
- errno = 0;
- int ret = mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
- if (ret == 0) {
- return 0;
- }
+ errno = 0;
+#ifdef WIN32
+ int ret = mkdir(path);
+ char path_sep = '\\';
+#else
+ int ret = mkdir(path,
+ S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+ char path_sep = '/';
+#endif
+ if (ret == 0) {
+ return 0;
+ }
- switch (errno) {
- case ENOENT: {
- char * found = strrchr(path, '/');
- if (!found) {
- return -1;
- }
- int len = found - path;
- char * dir = calloc(len + 1, sizeof(char));
- strncpy(dir, path, len);
- dir[len] = '\0';
- int res = make_dir(dir);
- free(dir);
- if (res < 0) {
- return -1;
- }
- return mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
+ switch (errno) {
+ case ENOENT: {
+ char * found = strrchr(path, path_sep);
+ if (!found) {
+ return -1;
}
- case EEXIST: {
- if (is_directory(path)) {
- return 0;
- }
- return -1;
+ int len = found - path;
+ char * dir = calloc(len + 1, sizeof(char));
+ strncpy(dir, path, len);
+ dir[len] = '\0';
+ int res = make_dir(dir);
+ free(dir);
+ if (res < 0) {
+ return -1;
}
- default:
- return -1;
+#ifdef WIN32
+ return mkdir(path);
+#else
+ return mkdir(path,
+ S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+#endif
+ }
+ case EEXIST: {
+ int ret = is_directory(path);
+ if (ret <= 0) {
+ return -1;
}
+ return 0;
+ }
+ default:
+ return -1;
+ }
}
char * get_current_working_directory() {
- char * cwd = (char *)malloc(PATH_MAX * sizeof(char));
- memset(cwd, 0, PATH_MAX);
- #ifdef WIN32
- if (_getcwd(cwd, PATH_MAX) != NULL)
- return cwd;
- #else
- if (getcwd(cwd, PATH_MAX) != NULL) {
- return cwd;
- }
- #endif
- free(cwd);
+ char * cwd = (char *) malloc(PATH_MAX * sizeof(char));
+ memset(cwd, 0, PATH_MAX);
+#ifdef WIN32
+ if (_getcwd(cwd, PATH_MAX) != NULL)
+ return cwd;
+#else
+ if (getcwd(cwd, PATH_MAX) != NULL) {
+ return cwd;
+ }
+#endif
+ free(cwd);
+ return NULL;
+}
+
+char * create_temp_directory(char * format) {
+#ifndef WIN32
+ if (!mkdtemp(format)) {
return NULL;
+ }
+ return copystr(format);
+#else
+ char * temp_dir_path = (char *)malloc(PATH_MAX);
+ memset(temp_dir_path, 0, PATH_MAX);
+ DWORD ret = GetTempPath(PATH_MAX, temp_dir_path);
+ if (ret <= PATH_MAX && ret != 0) {
+ CIDGenerator gen;
+ char uuid[37];
+ gen.implementation_ = CUUID_DEFAULT_IMPL;
+ generate_uuid(&gen, uuid);
+ uuid[36] = '\0';
+ strcat(temp_dir_path, ".");
+ strcat(temp_dir_path, uuid);
+ if (make_dir(temp_dir_path) < 0) {
+ return NULL;
+ }
+ return temp_dir_path;
+ }
Review comment:
1. If the returned temp path is exactly `PATH_MAX` (which I assume is the same as `MAX_PATH` as referred to by [MSDN](https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha)), then it doesn't fit in the buffer with the null terminator.
2. If the returned temp path is near `PATH_MAX` in length, then the `strcat` calls may cause buffer overflow. I think we may want to check for this and report failure if we can not concat the UUID to the path and still fit in `PATH_MAX`.
3. I suggest using the 'A' versions of the WINAPI functions to avoid dependency on the UNICODE macro.
```suggestion
const size_t uuid_buflen = 37;
const size_t max_temp_path_len = PATH_MAX - uuid_buflen - 2; // 2: "." and '\0'
char temp_dir_path[PATH_MAX + 1] = {0};
const DWORD ret = GetTempPathA(PATH_MAX + 1, temp_dir_path);
if (ret <= max_temp_path_len && ret != 0) {
CIDGenerator gen;
char uuid[uuid_buflen];
gen.implementation_ = CUUID_DEFAULT_IMPL;
generate_uuid(&gen, uuid);
uuid[uuid_buflen - 1] = '\0';
strcat(temp_dir_path, ".");
strcat(temp_dir_path, uuid);
if (make_dir(temp_dir_path) < 0) {
return NULL;
}
return copystr(temp_dir_path);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #772: MINIFICPP-1153 Fix failing CTailFile Tests
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #772:
URL: https://github.com/apache/nifi-minifi-cpp/pull/772
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on pull request #772: MINIFICPP-1153 Fix failing CTailFile Tests
Posted by GitBox <gi...@apache.org>.
arpadboda commented on pull request #772:
URL: https://github.com/apache/nifi-minifi-cpp/pull/772#issuecomment-644364584
Closing this as fixed in scope of 46517012d1cc94e36d4368a2ca1d806a7e48ac6e
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on pull request #772: MINIFICPP-1153 Fix failing CTailFile Tests
Posted by GitBox <gi...@apache.org>.
fgerlits commented on pull request #772:
URL: https://github.com/apache/nifi-minifi-cpp/pull/772#issuecomment-638214274
Hi Murtuza,
Just checking whether you are planning to finish this fix. No rush, but please let us know if you are still working on it, or you would like us to pick it up.
Thanks,
Ferenc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #772: MINIFICPP-1153 Fix failing CTailFile Tests
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #772:
URL: https://github.com/apache/nifi-minifi-cpp/pull/772#discussion_r420232627
##########
File path: nanofi/src/core/file_utils.c
##########
@@ -28,114 +28,222 @@
#include "core/string_utils.h"
#include "core/file_utils.h"
-#ifdef _MSC_VER
+#ifdef WIN32
#ifndef PATH_MAX
#define PATH_MAX 260
#endif
#endif
+#ifdef WIN32
+#define stat _stat
+#define mkdir _mkdir
+#endif
+
int is_directory(const char * path) {
- struct stat dir_stat;
- if (stat(path, &dir_stat) < 0) {
- return 0;
- }
- return S_ISDIR(dir_stat.st_mode);
+ struct stat dir_stat;
+ if (stat(path, &dir_stat) < 0) {
+ return -1;
+ }
+#ifdef WIN32
+ return dir_stat.st_mode & S_IFDIR;
+#else
+ return S_ISDIR(dir_stat.st_mode);
+#endif
}
-const char * get_separator(int force_posix)
-{
+const char * get_separator(int force_posix) {
#ifdef WIN32
- if (!force_posix) {
- return "\\";
- }
+ if (!force_posix) {
+ return "\\";
+ }
#endif
- return "/";
+ return "/";
}
char * concat_path(const char * parent, const char * child) {
- char * path = (char *)malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
- strcpy(path, parent);
- const char * sep = get_separator(0);
- strcat(path, sep);
- strcat(path, child);
- return path;
+ char * path = (char *) malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
+ strcpy(path, parent);
+ const char * sep = get_separator(0);
+ strcat(path, sep);
+ strcat(path, child);
+ return path;
}
+#ifndef WIN32
void remove_directory(const char * dir_path) {
-
- if (!is_directory(dir_path)) {
- if (unlink(dir_path) == -1) {
- printf("Could not remove file %s\n", dir_path);
- }
- return;
+ if (!is_directory(dir_path)) {
+ if (unlink(dir_path) == -1) {
+ printf("Could not remove file %s\n", dir_path);
}
+ return;
+ }
- uint64_t path_len = strlen(dir_path);
- struct dirent * dir;
- DIR * d = opendir(dir_path);
-
- while ((dir = readdir(d)) != NULL) {
- char * entry_name = dir->d_name;
- if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
- continue;
- }
- char * path = concat_path(dir_path, entry_name);
- remove_directory(path);
- free(path);
+ uint64_t path_len = strlen(dir_path);
+ struct dirent * dir;
+ DIR * d = opendir(dir_path);
+
+ while ((dir = readdir(d)) != NULL) {
+ char * entry_name = dir->d_name;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
+ continue;
}
+ char * path = concat_path(dir_path, entry_name);
+ remove_directory(path);
+ free(path);
+ }
+ rmdir(dir_path);
+
+ closedir(d);
+}
+#else
+void remove_directory(const char * dir_path) {
+ HANDLE hFind;
+ WIN32_FIND_DATA fd;
- rmdir(dir_path);
- closedir(d);
+ hFind = FindFirstFile(dir_path, &fd);
+ if (hFind == INVALID_HANDLE_VALUE) {
+ return;
+ }
+
+ if (fd.dwFileAttributes != FILE_ATTRIBUTE_DIRECTORY) {
+ DeleteFile(dir_path);
+ FindClose(hFind);
+ return;
+ }
+
+ char * path = concat_path(dir_path, "*");
+ HANDLE hFind1;
+ if ((hFind1 = FindFirstFile(path, &fd)) != INVALID_HANDLE_VALUE) {
+ do {
+ char * entry_name = fd.cFileName;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) continue;
+ char * entry_path = concat_path(dir_path, entry_name);
+ remove_directory(entry_path);
+ free(entry_path);
+ }while (FindNextFile(hFind1, &fd));
+ }
+ RemoveDirectory(dir_path);
+ FindClose(hFind);
+ FindClose(hFind1);
+ free(path);
}
+#endif
int make_dir(const char * path) {
- if (!path) return -1;
+ if (!path)
+ return -1;
- errno = 0;
- int ret = mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
- if (ret == 0) {
- return 0;
- }
+ errno = 0;
+#ifdef WIN32
+ int ret = mkdir(path);
+ char path_sep = '\\';
+#else
+ int ret = mkdir(path,
+ S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+ char path_sep = '/';
+#endif
Review comment:
`path_sep` could be initialized via `get_separator` to avoid duplicated logic. (Although it's quite unlikely that path separators are going to change anytime soon.)
Also, if the second argument of unix `mkdir` is wrapped to a new line, it should use continuation indentation.
```suggestion
#ifdef WIN32
const int ret = mkdir(path);
#else
const int ret = mkdir(path, S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
#endif /* WIN32 */
const char path_sep = get_separator(0)[0];
```
##########
File path: nanofi/src/core/file_utils.c
##########
@@ -28,114 +28,222 @@
#include "core/string_utils.h"
#include "core/file_utils.h"
-#ifdef _MSC_VER
+#ifdef WIN32
#ifndef PATH_MAX
#define PATH_MAX 260
#endif
#endif
+#ifdef WIN32
+#define stat _stat
+#define mkdir _mkdir
+#endif
+
int is_directory(const char * path) {
- struct stat dir_stat;
- if (stat(path, &dir_stat) < 0) {
- return 0;
- }
- return S_ISDIR(dir_stat.st_mode);
+ struct stat dir_stat;
+ if (stat(path, &dir_stat) < 0) {
+ return -1;
+ }
+#ifdef WIN32
+ return dir_stat.st_mode & S_IFDIR;
+#else
+ return S_ISDIR(dir_stat.st_mode);
+#endif
}
-const char * get_separator(int force_posix)
-{
+const char * get_separator(int force_posix) {
#ifdef WIN32
- if (!force_posix) {
- return "\\";
- }
+ if (!force_posix) {
+ return "\\";
+ }
#endif
- return "/";
+ return "/";
}
char * concat_path(const char * parent, const char * child) {
- char * path = (char *)malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
- strcpy(path, parent);
- const char * sep = get_separator(0);
- strcat(path, sep);
- strcat(path, child);
- return path;
+ char * path = (char *) malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
+ strcpy(path, parent);
+ const char * sep = get_separator(0);
+ strcat(path, sep);
+ strcat(path, child);
+ return path;
}
+#ifndef WIN32
void remove_directory(const char * dir_path) {
-
- if (!is_directory(dir_path)) {
- if (unlink(dir_path) == -1) {
- printf("Could not remove file %s\n", dir_path);
- }
- return;
+ if (!is_directory(dir_path)) {
+ if (unlink(dir_path) == -1) {
+ printf("Could not remove file %s\n", dir_path);
}
+ return;
+ }
- uint64_t path_len = strlen(dir_path);
- struct dirent * dir;
- DIR * d = opendir(dir_path);
-
- while ((dir = readdir(d)) != NULL) {
- char * entry_name = dir->d_name;
- if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
- continue;
- }
- char * path = concat_path(dir_path, entry_name);
- remove_directory(path);
- free(path);
+ uint64_t path_len = strlen(dir_path);
+ struct dirent * dir;
+ DIR * d = opendir(dir_path);
+
+ while ((dir = readdir(d)) != NULL) {
+ char * entry_name = dir->d_name;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
+ continue;
}
+ char * path = concat_path(dir_path, entry_name);
+ remove_directory(path);
+ free(path);
+ }
+ rmdir(dir_path);
+
+ closedir(d);
+}
+#else
+void remove_directory(const char * dir_path) {
+ HANDLE hFind;
+ WIN32_FIND_DATA fd;
- rmdir(dir_path);
- closedir(d);
+ hFind = FindFirstFile(dir_path, &fd);
+ if (hFind == INVALID_HANDLE_VALUE) {
+ return;
+ }
+
+ if (fd.dwFileAttributes != FILE_ATTRIBUTE_DIRECTORY) {
+ DeleteFile(dir_path);
+ FindClose(hFind);
+ return;
+ }
+
+ char * path = concat_path(dir_path, "*");
+ HANDLE hFind1;
+ if ((hFind1 = FindFirstFile(path, &fd)) != INVALID_HANDLE_VALUE) {
+ do {
+ char * entry_name = fd.cFileName;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) continue;
+ char * entry_path = concat_path(dir_path, entry_name);
+ remove_directory(entry_path);
+ free(entry_path);
+ }while (FindNextFile(hFind1, &fd));
+ }
+ RemoveDirectory(dir_path);
+ FindClose(hFind);
+ FindClose(hFind1);
+ free(path);
}
+#endif
int make_dir(const char * path) {
- if (!path) return -1;
+ if (!path)
+ return -1;
- errno = 0;
- int ret = mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
- if (ret == 0) {
- return 0;
- }
+ errno = 0;
+#ifdef WIN32
+ int ret = mkdir(path);
+ char path_sep = '\\';
+#else
+ int ret = mkdir(path,
+ S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+ char path_sep = '/';
+#endif
+ if (ret == 0) {
+ return 0;
+ }
- switch (errno) {
- case ENOENT: {
- char * found = strrchr(path, '/');
- if (!found) {
- return -1;
- }
- int len = found - path;
- char * dir = calloc(len + 1, sizeof(char));
- strncpy(dir, path, len);
- dir[len] = '\0';
- int res = make_dir(dir);
- free(dir);
- if (res < 0) {
- return -1;
- }
- return mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
+ switch (errno) {
+ case ENOENT: {
+ char * found = strrchr(path, path_sep);
+ if (!found) {
+ return -1;
}
- case EEXIST: {
- if (is_directory(path)) {
- return 0;
- }
- return -1;
+ int len = found - path;
+ char * dir = calloc(len + 1, sizeof(char));
+ strncpy(dir, path, len);
+ dir[len] = '\0';
Review comment:
Nothing important, just a few small things here:
- `sizeof(char)` is defined to be 1 by the standard (i.e. on all platforms)
- `calloc` initializes the allocated storage to zero, so the last line is redundant. Alternatively, `(char*)malloc(len+1)` could be used, leaving the last line intact.
- While implicit `void*` to other pointer conversion is valid C (but not valid C++), I think it's good practice to not rely on this and add an explicit cast to the appropriate type, as done in every other allocation in the file.
```suggestion
const ptrdiff_t len = found - path;
char * const dir = (char*)malloc(len + 1);
strncpy(dir, path, len);
dir[len] = '\0';
```
##########
File path: nanofi/src/core/file_utils.c
##########
@@ -28,114 +28,222 @@
#include "core/string_utils.h"
#include "core/file_utils.h"
-#ifdef _MSC_VER
+#ifdef WIN32
#ifndef PATH_MAX
#define PATH_MAX 260
#endif
#endif
+#ifdef WIN32
+#define stat _stat
+#define mkdir _mkdir
+#endif
+
int is_directory(const char * path) {
- struct stat dir_stat;
- if (stat(path, &dir_stat) < 0) {
- return 0;
- }
- return S_ISDIR(dir_stat.st_mode);
+ struct stat dir_stat;
+ if (stat(path, &dir_stat) < 0) {
+ return -1;
+ }
+#ifdef WIN32
+ return dir_stat.st_mode & S_IFDIR;
+#else
+ return S_ISDIR(dir_stat.st_mode);
+#endif
}
-const char * get_separator(int force_posix)
-{
+const char * get_separator(int force_posix) {
#ifdef WIN32
- if (!force_posix) {
- return "\\";
- }
+ if (!force_posix) {
+ return "\\";
+ }
#endif
- return "/";
+ return "/";
}
char * concat_path(const char * parent, const char * child) {
- char * path = (char *)malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
- strcpy(path, parent);
- const char * sep = get_separator(0);
- strcat(path, sep);
- strcat(path, child);
- return path;
+ char * path = (char *) malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
+ strcpy(path, parent);
+ const char * sep = get_separator(0);
+ strcat(path, sep);
+ strcat(path, child);
+ return path;
}
+#ifndef WIN32
void remove_directory(const char * dir_path) {
-
- if (!is_directory(dir_path)) {
- if (unlink(dir_path) == -1) {
- printf("Could not remove file %s\n", dir_path);
- }
- return;
+ if (!is_directory(dir_path)) {
+ if (unlink(dir_path) == -1) {
+ printf("Could not remove file %s\n", dir_path);
}
+ return;
+ }
- uint64_t path_len = strlen(dir_path);
- struct dirent * dir;
- DIR * d = opendir(dir_path);
-
- while ((dir = readdir(d)) != NULL) {
- char * entry_name = dir->d_name;
- if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
- continue;
- }
- char * path = concat_path(dir_path, entry_name);
- remove_directory(path);
- free(path);
+ uint64_t path_len = strlen(dir_path);
+ struct dirent * dir;
+ DIR * d = opendir(dir_path);
+
+ while ((dir = readdir(d)) != NULL) {
+ char * entry_name = dir->d_name;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
+ continue;
}
+ char * path = concat_path(dir_path, entry_name);
+ remove_directory(path);
+ free(path);
+ }
+ rmdir(dir_path);
+
+ closedir(d);
+}
+#else
+void remove_directory(const char * dir_path) {
+ HANDLE hFind;
+ WIN32_FIND_DATA fd;
- rmdir(dir_path);
- closedir(d);
+ hFind = FindFirstFile(dir_path, &fd);
+ if (hFind == INVALID_HANDLE_VALUE) {
+ return;
+ }
+
+ if (fd.dwFileAttributes != FILE_ATTRIBUTE_DIRECTORY) {
+ DeleteFile(dir_path);
+ FindClose(hFind);
+ return;
+ }
+
+ char * path = concat_path(dir_path, "*");
+ HANDLE hFind1;
+ if ((hFind1 = FindFirstFile(path, &fd)) != INVALID_HANDLE_VALUE) {
+ do {
+ char * entry_name = fd.cFileName;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) continue;
+ char * entry_path = concat_path(dir_path, entry_name);
+ remove_directory(entry_path);
+ free(entry_path);
+ }while (FindNextFile(hFind1, &fd));
+ }
+ RemoveDirectory(dir_path);
+ FindClose(hFind);
+ FindClose(hFind1);
+ free(path);
}
+#endif
int make_dir(const char * path) {
- if (!path) return -1;
+ if (!path)
+ return -1;
- errno = 0;
- int ret = mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
- if (ret == 0) {
- return 0;
- }
+ errno = 0;
+#ifdef WIN32
+ int ret = mkdir(path);
+ char path_sep = '\\';
+#else
+ int ret = mkdir(path,
+ S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+ char path_sep = '/';
+#endif
+ if (ret == 0) {
+ return 0;
+ }
- switch (errno) {
- case ENOENT: {
- char * found = strrchr(path, '/');
- if (!found) {
- return -1;
- }
- int len = found - path;
- char * dir = calloc(len + 1, sizeof(char));
- strncpy(dir, path, len);
- dir[len] = '\0';
- int res = make_dir(dir);
- free(dir);
- if (res < 0) {
- return -1;
- }
- return mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
+ switch (errno) {
+ case ENOENT: {
+ char * found = strrchr(path, path_sep);
+ if (!found) {
+ return -1;
}
- case EEXIST: {
- if (is_directory(path)) {
- return 0;
- }
- return -1;
+ int len = found - path;
+ char * dir = calloc(len + 1, sizeof(char));
+ strncpy(dir, path, len);
+ dir[len] = '\0';
+ int res = make_dir(dir);
+ free(dir);
+ if (res < 0) {
+ return -1;
}
- default:
- return -1;
+#ifdef WIN32
+ return mkdir(path);
+#else
+ return mkdir(path,
+ S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+#endif
+ }
+ case EEXIST: {
+ int ret = is_directory(path);
+ if (ret <= 0) {
+ return -1;
}
+ return 0;
+ }
+ default:
+ return -1;
+ }
}
char * get_current_working_directory() {
- char * cwd = (char *)malloc(PATH_MAX * sizeof(char));
- memset(cwd, 0, PATH_MAX);
- #ifdef WIN32
- if (_getcwd(cwd, PATH_MAX) != NULL)
- return cwd;
- #else
- if (getcwd(cwd, PATH_MAX) != NULL) {
- return cwd;
- }
- #endif
- free(cwd);
+ char * cwd = (char *) malloc(PATH_MAX * sizeof(char));
+ memset(cwd, 0, PATH_MAX);
+#ifdef WIN32
+ if (_getcwd(cwd, PATH_MAX) != NULL)
+ return cwd;
+#else
+ if (getcwd(cwd, PATH_MAX) != NULL) {
+ return cwd;
+ }
+#endif
+ free(cwd);
+ return NULL;
+}
+
+char * create_temp_directory(char * format) {
+#ifndef WIN32
+ if (!mkdtemp(format)) {
return NULL;
+ }
+ return copystr(format);
+#else
+ char * temp_dir_path = (char *)malloc(PATH_MAX);
+ memset(temp_dir_path, 0, PATH_MAX);
+ DWORD ret = GetTempPath(PATH_MAX, temp_dir_path);
+ if (ret <= PATH_MAX && ret != 0) {
+ CIDGenerator gen;
+ char uuid[37];
+ gen.implementation_ = CUUID_DEFAULT_IMPL;
+ generate_uuid(&gen, uuid);
+ uuid[36] = '\0';
+ strcat(temp_dir_path, ".");
+ strcat(temp_dir_path, uuid);
+ if (make_dir(temp_dir_path) < 0) {
+ return NULL;
+ }
+ return temp_dir_path;
+ }
Review comment:
1. If the returned temp path is exactly `PATH_MAX` (which I assume is the same as `MAX_PATH` as referred to by [MSDN](https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha)), then it doesn't fit in the buffer with the null terminator.
2. If the returned temp path is near `PATH_MAX` in length, then the `strcat` calls may cause buffer overflow. I think we may want to check for this and report failure if we can not concat the UUID to the path and still fit in `PATH_MAX`.
3. I suggest using the 'A' versions of the WINAPI functions to avoid dependency on the UNICODE macro.
```suggestion
const size_t uuid_strlen = 37;
const size_t max_temp_path_len = PATH_MAX - uuid_strlen - 2; // 2: "." and '\0'
char temp_dir_path[PATH_MAX + 1] = {0};
const DWORD ret = GetTempPathA(PATH_MAX + 1, temp_dir_path);
if (ret <= max_temp_path_len && ret != 0) {
CIDGenerator gen;
char uuid[uuid_strlen];
gen.implementation_ = CUUID_DEFAULT_IMPL;
generate_uuid(&gen, uuid);
uuid[uuid_strlen - 1] = '\0';
strcat(temp_dir_path, ".");
strcat(temp_dir_path, uuid);
if (make_dir(temp_dir_path) < 0) {
return NULL;
}
return copystr(temp_dir_path);
}
```
##########
File path: nanofi/src/core/file_utils.c
##########
@@ -28,114 +28,222 @@
#include "core/string_utils.h"
#include "core/file_utils.h"
-#ifdef _MSC_VER
+#ifdef WIN32
#ifndef PATH_MAX
#define PATH_MAX 260
#endif
#endif
+#ifdef WIN32
+#define stat _stat
+#define mkdir _mkdir
+#endif
+
int is_directory(const char * path) {
- struct stat dir_stat;
- if (stat(path, &dir_stat) < 0) {
- return 0;
- }
- return S_ISDIR(dir_stat.st_mode);
+ struct stat dir_stat;
+ if (stat(path, &dir_stat) < 0) {
+ return -1;
+ }
+#ifdef WIN32
+ return dir_stat.st_mode & S_IFDIR;
+#else
+ return S_ISDIR(dir_stat.st_mode);
+#endif
}
-const char * get_separator(int force_posix)
-{
+const char * get_separator(int force_posix) {
#ifdef WIN32
- if (!force_posix) {
- return "\\";
- }
+ if (!force_posix) {
+ return "\\";
+ }
#endif
- return "/";
+ return "/";
}
char * concat_path(const char * parent, const char * child) {
- char * path = (char *)malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
- strcpy(path, parent);
- const char * sep = get_separator(0);
- strcat(path, sep);
- strcat(path, child);
- return path;
+ char * path = (char *) malloc((strlen(parent) + strlen(child) + 2) * sizeof(char));
+ strcpy(path, parent);
+ const char * sep = get_separator(0);
+ strcat(path, sep);
+ strcat(path, child);
+ return path;
}
+#ifndef WIN32
void remove_directory(const char * dir_path) {
-
- if (!is_directory(dir_path)) {
- if (unlink(dir_path) == -1) {
- printf("Could not remove file %s\n", dir_path);
- }
- return;
+ if (!is_directory(dir_path)) {
+ if (unlink(dir_path) == -1) {
+ printf("Could not remove file %s\n", dir_path);
}
+ return;
+ }
- uint64_t path_len = strlen(dir_path);
- struct dirent * dir;
- DIR * d = opendir(dir_path);
-
- while ((dir = readdir(d)) != NULL) {
- char * entry_name = dir->d_name;
- if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
- continue;
- }
- char * path = concat_path(dir_path, entry_name);
- remove_directory(path);
- free(path);
+ uint64_t path_len = strlen(dir_path);
+ struct dirent * dir;
+ DIR * d = opendir(dir_path);
+
+ while ((dir = readdir(d)) != NULL) {
+ char * entry_name = dir->d_name;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) {
+ continue;
}
+ char * path = concat_path(dir_path, entry_name);
+ remove_directory(path);
+ free(path);
+ }
+ rmdir(dir_path);
+
+ closedir(d);
+}
+#else
+void remove_directory(const char * dir_path) {
+ HANDLE hFind;
+ WIN32_FIND_DATA fd;
- rmdir(dir_path);
- closedir(d);
+ hFind = FindFirstFile(dir_path, &fd);
+ if (hFind == INVALID_HANDLE_VALUE) {
+ return;
+ }
+
+ if (fd.dwFileAttributes != FILE_ATTRIBUTE_DIRECTORY) {
+ DeleteFile(dir_path);
+ FindClose(hFind);
+ return;
+ }
+
+ char * path = concat_path(dir_path, "*");
+ HANDLE hFind1;
+ if ((hFind1 = FindFirstFile(path, &fd)) != INVALID_HANDLE_VALUE) {
+ do {
+ char * entry_name = fd.cFileName;
+ if (!strcmp(entry_name, ".") || !strcmp(entry_name, "..")) continue;
+ char * entry_path = concat_path(dir_path, entry_name);
+ remove_directory(entry_path);
+ free(entry_path);
+ }while (FindNextFile(hFind1, &fd));
+ }
+ RemoveDirectory(dir_path);
+ FindClose(hFind);
+ FindClose(hFind1);
+ free(path);
}
+#endif
int make_dir(const char * path) {
- if (!path) return -1;
+ if (!path)
+ return -1;
- errno = 0;
- int ret = mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
- if (ret == 0) {
- return 0;
- }
+ errno = 0;
+#ifdef WIN32
+ int ret = mkdir(path);
+ char path_sep = '\\';
+#else
+ int ret = mkdir(path,
+ S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+ char path_sep = '/';
+#endif
+ if (ret == 0) {
+ return 0;
+ }
- switch (errno) {
- case ENOENT: {
- char * found = strrchr(path, '/');
- if (!found) {
- return -1;
- }
- int len = found - path;
- char * dir = calloc(len + 1, sizeof(char));
- strncpy(dir, path, len);
- dir[len] = '\0';
- int res = make_dir(dir);
- free(dir);
- if (res < 0) {
- return -1;
- }
- return mkdir(path, S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
+ switch (errno) {
+ case ENOENT: {
+ char * found = strrchr(path, path_sep);
+ if (!found) {
+ return -1;
}
- case EEXIST: {
- if (is_directory(path)) {
- return 0;
- }
- return -1;
+ int len = found - path;
+ char * dir = calloc(len + 1, sizeof(char));
+ strncpy(dir, path, len);
+ dir[len] = '\0';
+ int res = make_dir(dir);
+ free(dir);
+ if (res < 0) {
+ return -1;
}
- default:
- return -1;
+#ifdef WIN32
+ return mkdir(path);
+#else
+ return mkdir(path,
+ S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
Review comment:
I think this shouldn't be wrapped, or if wrapped, should use continuation indentation.
```suggestion
return mkdir(path, S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org