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

internal/sys: add constants #1537

Merged
merged 2 commits into from
Aug 6, 2024
Merged

internal/sys: add constants #1537

merged 2 commits into from
Aug 6, 2024

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Aug 6, 2024

internal/sys: generate constants from unnamed enums

bpf.h defines a bunch of constants as unnamed enums. This makes them tricky
to pull out of vmlinux BTF, so instead we rely on the internal/unix shim.
This causes a small maintenance burden whenever we need a new flag, but more
importantly it makes some upcoming cross platform work harder.

Pull the constant definitions out of vmlinux BTF instead of relying on 
package unix. Unfortunately a bunch of the flags are re-used. This is most
noticeable for MapFlags, for which the enum in the kernel has become a bit
of a grab bag. Instead of trying hard to wrap everything into a neat type we
just generate untyped constants and get rid of MapFlags.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

internal/sys: add DEFINEd constants

Some constants in bpf.h are defined using the C preprocessor and are 
therefore not available in BTF. Add them manually.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

@github-actions github-actions bot added the breaking-change Changes exported API label Aug 6, 2024
@@ -227,7 +227,7 @@ func init() {
}

// MapFlags document which flags may be feature probed.
type MapFlags = sys.MapFlags
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the breaking change the linter is complaining about. I don't think this should make a difference in practice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, should be fine since external callers never had access to sys.MapFlags anyway.

@lmb lmb marked this pull request as ready for review August 6, 2024 13:40
@lmb lmb requested review from florianl, mmat11, rgo3 and a team as code owners August 6, 2024 13:40
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

One suggestion, looks fine otherwise!

internal/cmd/gentypes/main.go Outdated Show resolved Hide resolved
@ti-mo ti-mo removed the breaking-change Changes exported API label Aug 6, 2024
@github-actions github-actions bot added the breaking-change Changes exported API label Aug 6, 2024
lmb added 2 commits August 6, 2024 16:15
bpf.h defines a bunch of constants as unnamed enums. This makes them
tricky to pull out of vmlinux BTF, so instead we rely on the internal/unix
shim. This causes a small maintenance burden whenever we need a new flag,
but more importantly it makes some upcoming cross platform work harder.

Pull the constant definitions out of vmlinux BTF instead of relying on
package unix. Unfortunately a bunch of the flags are re-used.
This is most noticeable for MapFlags, for which the enum in the kernel
has become a bit of a grab bag. Instead of trying hard to wrap everything
into a neat type we just generate untyped constants and get rid of MapFlags.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Some constants in bpf.h are defined using the C preprocessor and are
therefore not available in BTF. Add them manually.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb lmb merged commit 3403595 into cilium:main Aug 6, 2024
17 checks passed
@lmb lmb deleted the sys-constants branch August 6, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes exported API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants