Better user-group creating in postinst, BaseDaemon::getDefaultCorePath [#METR-23811] #295
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexey-milovidov
requested changes
Dec 31, 2016
| @@ -188,6 +188,9 @@ static std::string getCanonicalPath(std::string && path) | |||
| return path; | |||
| } | |||
|
|
|||
| std::string Server::getDefaultCorePath() { | |||
| @@ -701,6 +703,10 @@ void BaseDaemon::closeLogs() | |||
| logger().warning("Logging to console but received signal to close log file (ignoring)."); | |||
| } | |||
|
|
|||
| std::string BaseDaemon::getDefaultCorePath () { | |||
| @@ -61,6 +61,8 @@ class Server : public BaseDaemon | |||
| private: | |||
|
|
|||
| void attachSystemTables(const std::string & path, bool has_zookeeper) const; | |||
|
|
|||
| std::string getDefaultCorePath() override; | |||
| @@ -436,19 +436,21 @@ static void terminate_handler() | |||
| } | |||
|
|
|||
|
|
|||
| static std::string createDirectory(const std::string & _path) | |||
| static std::string createDirectory(const std::string & _path, bool strip_file = 1) | |||
There was a problem hiding this comment.
This is ancient code. Why not to throw away and use Poco::File::createDirectories instead?
| if (core_path.empty()) | ||
| core_path = getDefaultCorePath(); | ||
|
|
||
| try { |
There was a problem hiding this comment.
No need to catch. Let it fail.
| core_path = !log_path.empty() ? log_path : "/opt/"; | ||
| try { | ||
| createDirectory(core_path, false); | ||
| } catch (...) { } |
| } catch (...) { } | ||
|
|
||
| Poco::File cores = core_path; | ||
| if ( !( cores.exists() && cores.isDirectory() ) ) { |
There was a problem hiding this comment.
No need for whitespace in inner side of parens (style).
|
|
||
| try { | ||
| createDirectory(core_path, false); | ||
| } catch (...) { } |
There was a problem hiding this comment.
Silent catching of all exceptions is prohibited in style.
| @@ -34,7 +34,7 @@ function make_control { | |||
| do | |||
| case "$DAEMON_PKG" in | |||
| 'clickhouse-server' ) | |||
| add_daemon_impl clickhouse-server-base '' 'clickhouse-server binary' | |||
| add_daemon_impl clickhouse-server-base 'adduser' 'clickhouse-server binary' | |||
There was a problem hiding this comment.
I read this just now:
useradd is a low level utility for adding users. On Debian, administrators should usually use adduser(8) instead.
What if people are installing debian package on non-debian system (this way is used in practice)? Dependencies are cost too much... I am not sure that adduser gives enough comparing to useradd. BTW, I call these kind of programs "trash-scripts".
| #mkdir -p /var/log/clickhouse | ||
| #chmod 1775 /var/log/clickhouse | ||
| #chown root:metrika /var/log/clickhouse | ||
| fi |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.