8000 Check pid file by datawolf · Pull Request #1147 · opencontainers/runc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Check pid file #1147

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

Merged
merged 1 commit into from
Oct 26, 2016
Merged

Check pid file #1147

merged 1 commit into from
Oct 26, 2016

Conversation

datawolf
Copy link
Contributor
@datawolf datawolf commented Oct 25, 2016

At #1132 (review), It is necessary to make sure that pid-file contains the right information.

This patch employ jq and runc state command to achieve it.

Signed-off-by: Wang Long long.wanglong@huawei.com

@datawolf datawolf force-pushed the check-pid-file branch 2 times, most recently from faacdf7 to 9723638 Compare October 25, 2016 07:34
…ight information

Signed-off-by: Wang Long <long.wanglong@huawei.com>
@cyphar
Copy link
Member
cyphar commented Oct 25, 2016

LGTM. Thanks. 😸

Approved with PullApprove

@@ -51,7 +51,7 @@ function teardown() {

run cat pid.txt
[ "$status" -eq 0 ]
[[ ${lines[0]} =~ [0-9]+ ]]
Copy link
Member

Choose a reason for hiding this comment

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

Why delete the digit test here.... but not below.

@@ -42,6 +42,7 @@ function teardown() {
run cat pid.txt
[ "$status" -eq 0 ]
[[ ${lines[0]} =~ [0-9]+ ]]
Copy link
Member

Choose a reason for hiding this comment

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

here?

Copy link
Member
@cyphar cyphar Oct 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to de 8000 scribe this comment to others. Learn more.

Because this check is negative (is the pid not the main container pid) while the other checks are positive (is the pid the main container pid). So we need to retain the "is this actually a number" check.

Copy link
Member

Choose a reason for hiding this comment

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

thx missed the !=

LGTM

@hqhq
Copy link
Contributor
hqhq commented Oct 26, 2016

LGTM

Approved with PullApprove

@hqhq hqhq merged commit a08733b into opencontainers:master Oct 26, 2016
@datawolf datawolf deleted the check-pid-file branch November 8, 2016 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0