-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix directories handling #71
Conversation
623c27d
to
7f14f3e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes suggested but none required.
config-vdms.json
Outdated
@@ -4,15 +4,11 @@ | |||
{ | |||
// Network | |||
"port": 55555, // Default is 55555 | |||
"max_simultaneous_clients": 20, // Default is 500 | |||
"max_simultaneous_clients": 100, // Default is 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the comment not match the value?
And why would the comment be needed at all, if it did match the value?
config-vdms.json
Outdated
"tdb_path": "db/images/tiledb/tdb/", | ||
"blob_path":"db/blobs/", | ||
// For more path configuration options, check documentation. | ||
"db_root_path": "db", // Default is "db" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment useful?
(This also applies in other places the comment simply states the value in the initializer.)
src/VDMSConfig.cc
Outdated
// IMAGES - TDB | ||
path_tdb = path_images + "/" + DEFAULT_PATH_TDB; | ||
path_tdb = get_string_value(PARAM_DB_TDB, path_tdb); | ||
check_or_create(path_tdb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a nested block here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need, it just looked better. but you are right, no good reason.
tests/python/config-tests.json
Outdated
@@ -3,5 +3,8 @@ | |||
// Sets database paths and other parameters | |||
{ | |||
// Network | |||
"port": 55557 // Default is 55555 | |||
"port": 55557, // Default is 55555 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment doesn't match the value.
src/VDMSConfig.cc
Outdated
int success = 0; | ||
if (dir_exist(path)) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to call dir_exist. Create_dir checks and returns success if the directory already exists.
Dir_exist returns failure if the directory exists and is searchable but not readable, even though that may not actually be a problem.
Also I think it returns success if the directory is readable but not searchable, which would actually be a problem.
src/VDMSConfig.h
Outdated
#include <sys/stat.h> | ||
#include <unistd.h> | ||
#include <dirent.h> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this module not meant to be portable?
7f14f3e
to
0be89f7
Compare
Fix #12 and #38