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

specs-go: add consts for seccomp flags #1108

Closed
wants to merge 1 commit into from

Conversation

thaJeztah
Copy link
Member

Relates to moby/moby#42619

Commit d1ef109 (#1018) added the ability to specify
flags that must be passed to seccomp(2) when installing the filter, and
defined an enum in the specification.

This patch adds corresponding consts for the Go implementation of the Spec.

Commit d1ef109 added the ability to specify
flags that must be passed to seccomp(2) when installing the filter, and
defined an enum in the specification.

This patch adds corresponding consts for the Go implementation of the Spec.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@giuseppe @tianon @crosbymichael PTAL

/cc @sporksmith

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

@tianon
Copy link
Member

tianon commented Jul 17, 2021

I'm a little worried about how generic the naming convention here is -- I realize we did the same thing below with ArchFoo, but it feels like maybe we should use something a tad more specific since "flags" seem like something the specs might have others of outside Linux / seccomp. Thoughts?

@thaJeztah
Copy link
Member Author

I'm a little worried about how generic the naming convention here is -- I realize we did the same thing below with ArchFoo, but it feels like maybe we should use something a tad more specific since "flags" seem like something the specs might have others of outside Linux / seccomp. Thoughts?

Yes, I was actually looking at that as well; I did not add a prefix, because all existing ones did not have so, but all of these are in a single "namespace", and don't have prefixes to identify what they're for.

We could decide to

  • add Seccomp or Scmp prefixes to all of them
  • move them to a seccomp or seccompoptions dir / package

Either of the above would be a "breaking change", but we can of course add aliases and deprecate the existing ones.

Let me know what you think

@@ -613,6 +613,13 @@ type Arch string
// LinuxSeccompFlag is a flag to pass to seccomp(2).
type LinuxSeccompFlag string

// LinuxSeccompFlag options
const (
FlagTSync LinuxSeccompFlag = "SECCOMP_FILTER_FLAG_TSYNC"
Copy link
Member

@rata rata Jul 30, 2021

Choose a reason for hiding this comment

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

I'm not sure we want to add a const for this. This flag should probably not be used, as we asked here: #1077.

Maybe it is better to not facilitate using it, or even better mark it as deprecated in the spec? (the latter is out of scope for this PR, but a follow-up PR to discuss it would be great IMHO).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point; I'm fine with removing this one; I wasn't aware of the other discussion 👍

@tianon
Copy link
Member

tianon commented Jul 18, 2022

Closing in favor of #1138 🙇 ❤️

@tianon tianon closed this Jul 18, 2022
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.

4 participants