Skip to content
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

podman service: close duplicated /dev/null fd #16901

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Dec 20, 2022

Since we open /dev/null to set it as STDIN we can close it after the dup2() call. Using defer is not good enough since this function will never exit since the http server will block. This is not a problem but it reduces the open fds from the service by one.

[NO NEW TESTS NEEDED]

Does this PR introduce a user-facing change?

None

Since we open /dev/null to set it as STDIN we can close it after the
dup2() call. Using defer is not good enough since this function will
never exit since the http server will block. This is not a problem but
it reduces the open fds from the service by one.

[NO NEW TESTS NEEDED]

Signed-off-by: Paul Holzinger <[email protected]>
@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 20, 2022
@rhatdan
Copy link
Member

rhatdan commented Dec 20, 2022

LGTM

@rhatdan
Copy link
Member

rhatdan commented Dec 21, 2022

@vrothberg @giuseppe @flouthoc @containers/podman-maintainers PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2022
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM
/lgtm
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Luap99,flouthoc,giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 5211446 into containers:main Dec 22, 2022
@Luap99 Luap99 deleted the service-close-fd branch January 3, 2023 15:50
vrothberg added a commit to vrothberg/libpod that referenced this pull request Feb 13, 2023
Install the signal proxy before attaching to/starting the container to
make sure there's no race-condition as revealed in the failing start/run
tests in containers#16901.  The tests had the valid expectation that signal
forwarding works once the container is running.

Further update the tests to account for the attach test where the
expectation is that signal forwarding works once Podman has attached to
container (or even before).

Fixes: containers#16901
Signed-off-by: Valentin Rothberg <[email protected]>
Signed-off-by: Ed Santiago <[email protected]>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/podman that referenced this pull request Feb 13, 2023
Install the signal proxy before attaching to/starting the container to
make sure there's no race-condition as revealed in the failing start/run
tests in containers#16901.  The tests had the valid expectation that signal
forwarding works once the container is running.

Further update the tests to account for the attach test where the
expectation is that signal forwarding works once Podman has attached to
container (or even before).

Fixes: containers#16901
Signed-off-by: Valentin Rothberg <[email protected]>
Signed-off-by: Ed Santiago <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants