Skip to content

Fix directories handling#71

Merged
luisremis merged 1 commit intodevelopfrom
fix_default_paths
Jan 15, 2019
Merged

Fix directories handling#71
luisremis merged 1 commit intodevelopfrom
fix_default_paths

Conversation

@luisremis
Copy link
Copy Markdown
Contributor

Fix #12 and #38

philiplantz
philiplantz previously approved these changes Jan 15, 2019
Copy link
Copy Markdown

@philiplantz philiplantz left a 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
// Network
"port": 55555, // Default is 55555
"max_simultaneous_clients": 20, // Default is 500
"max_simultaneous_clients": 100, // Default is 500
Copy link
Copy Markdown

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"
Copy link
Copy Markdown

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.)

// IMAGES - TDB
path_tdb = path_images + "/" + DEFAULT_PATH_TDB;
path_tdb = get_string_value(PARAM_DB_TDB, path_tdb);
check_or_create(path_tdb);
Copy link
Copy Markdown

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?

Copy link
Copy Markdown
Contributor Author

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.

{
// Network
"port": 55557 // Default is 55555
"port": 55557, // Default is 55555
Copy link
Copy Markdown

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.

int success = 0;
if (dir_exist(path)) {
return;
}
Copy link
Copy Markdown

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>

Copy link
Copy Markdown

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?

@luisremis luisremis merged commit 7ac412a into develop Jan 15, 2019
@luisremis luisremis mentioned this pull request Jan 15, 2019
@luisremis luisremis deleted the fix_default_paths branch January 15, 2019 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants