Skip to content

Check pid file contents inside clickhouse server#1498

Merged
alexey-milovidov merged 8 commits intoClickHouse:masterfrom
proller:fix8
Nov 16, 2017
Merged

Check pid file contents inside clickhouse server#1498
alexey-milovidov merged 8 commits intoClickHouse:masterfrom
proller:fix8

Conversation

@proller
Copy link
Copy Markdown
Contributor

@proller proller commented Nov 15, 2017

No description provided.


bool is_pid_running(pid_t pid)
{
if (0 == kill(pid, 0))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If sig is 0, then no signal is sent, but existence and permission checks are still performed; this can be used to check for the existence of a process ID or process group ID that the caller is permitted to signal.

Excessive check. There is the possibility that the process is running from different user.


bool is_pid_running(pid_t pid)
{
if (0 == kill(pid, 0))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No error checks.

);
}

bool is_pid_running(pid_t pid)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style.

std::ifstream in(file);
if (in.good())
{
in >> pid_read;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No error checks.

Why don't you use ReadBuffer?

throw Poco::Exception("Pid file exists and program running with pid = " + std::to_string(pid_read) + ", should not start daemon.");
}
}
std::cerr << "Old pid file exists (with pid = " << pid_read << "), removing.";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing eol.

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