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

BUG: define ActKillThread equal to ActKill, and deprecated ActKill #90

Closed
wants to merge 2 commits into from

Conversation

thaJeztah
Copy link
Contributor

These constants are equal in libseccomp, with ActKillThread being a more
precise name for the previous behavior, but Go definitions were defined separately
in 24f2937.

This resulted in dead code that never executed due to identical case statements
in switch. Go can usually detect these error cases and refuses to build but
for some reason this detection doesn’t work with cgo+gcc.

Clang detects the equal constants correctly and therefore libseccomp-golang builds
with clang broke after ActKillThread was added making it impossible for us to
build the latest runc. https://gist.github.com/tonistiigi/ab6ef71781b7e3c5cabcb71a286a9243
is a simple example showing this condition where builds with clang fail.

In order to fix the clang build only removal of the switch case is needed. But I
assumed that the setter/getter logic is supposed to work for ActKillThread
as well and the only way to ensure that is to set them equal like they are in C.

tonistiigi and others added 2 commits April 22, 2022 12:10
These constants are equal in libseccomp but Go definitions
were defined separately. This resulted in dead code that
never executed due to identical case statements in switch.
Go can usually detect these error cases and refuses to build
but for some reason this detection doesn’t work with cgo+gcc.
Clang detects the equal constants correctly and therefore
libseccomp-golang builds with clang broke after ActKillThread
was added.

In order to fix the clang build only removal of the
switch case is needed. But I assumed that the setter/getter
logic is supposed to work for ActKillThread as well
and only way to ensure that is to set them equal like they
are in C.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Contributor Author

@tonistiigi @pcmoore @kolyshkin PTAL (I see there's no CI running in this repo, so could use the eyes to double-check this is good)

@thaJeztah
Copy link
Contributor Author

oh, never mind; I was typing too fast; it's just waiting approval to run 😂

Copy link

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Contributor Author

oh, let me ping @drakenclimber as well (I see I forgot) as they reviewed the original PR; ptal 🤗

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM, and also addresses #85 (comment)

@kolyshkin
Copy link
Contributor

@drakenclimber @pcmoore PTAL 🙏🏻

Copy link
Member

@drakenclimber drakenclimber left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @thaJeztah

Acked-by: Tom Hromatka <tom.hromatka@oracle.com>

@pcmoore pcmoore changed the title Fix: define ActKillThread equal to ActKill, and deprecated ActKill BUG: define ActKillThread equal to ActKill, and deprecated ActKill May 2, 2022
@pcmoore
Copy link
Member

pcmoore commented May 2, 2022

Merged at HEAD f33da4d, thank you!

@pcmoore pcmoore closed this May 2, 2022
@thaJeztah
Copy link
Contributor Author

Thank you!

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.

6 participants