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

map: detect map Name and PinPath separately #414

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Conversation

hao-lee
Copy link
Contributor

@hao-lee hao-lee commented Sep 9, 2021

libbpf uses /sys/fs/bpf as the default pinpath if the user doesn't specify
one. We'd better keep consistent with it.

static int build_map_pin_path(struct bpf_map *map, const char *path)
{
...
	if (!path)
		path = "/sys/fs/bpf";
...
}

Signed-off-by: Hao Lee haolee@didiglobal.com

@lmb
Copy link
Collaborator

lmb commented Sep 9, 2021

Hi @hao-lee,

I think this is not a safe default: /sys/fs/bpf is shared across the system, so using it as a default path will lead to conflicts if multiple projects using this library want to pin maps with the same name. For that reason, it's best to always use a subdirectory of /sys/fs/bpf.

If we want to tackle this in the library we need to come up with a safer default. Maybe it's possible to derive a subdirectory using os.Args[0]? This needs some thinking through of corner cases, etc. For example, what happens when using go run file.go like you can do in the examples?

@hao-lee
Copy link
Contributor Author

hao-lee commented Sep 9, 2021

Hello,

I think this is not a safe default: /sys/fs/bpf is shared across the system, so using it as a default path will lead to conflicts if multiple projects using this library want to pin maps with the same name. For that reason, it's best to always use a subdirectory of /sys/fs/bpf.

I think maybe it's the responsibility of the eBPF program developer to make sure the map's name is unique.

If we want to tackle this in the library we need to come up with a safer default. Maybe it's possible to derive a subdirectory using os.Args[0]?

This is indeed a solution if we want to avoid conflicts as soon as possible, although os.Args[0] is not foolproof.


If the developer doesn't configure the map's PinPath attribute in the Go code, the program will fail to load because of the missing PinPath. I think this behavior is too mandatory, especially for some beginners who only want to fire up a runnable eBPF program as simple as possible. 😅

@lmb
Copy link
Collaborator

lmb commented Sep 10, 2021

I think maybe it's the responsibility of the eBPF program developer to make sure the map's name is unique.

From my experience this unfortunately doesn't happen.

I think this behavior is too mandatory, especially for some beginners who only want to fire up a runnable eBPF program as simple as possible.

Will beginners start by pinning maps?

Maybe we can improve the error message?

@hao-lee
Copy link
Contributor Author

hao-lee commented Sep 10, 2021

Maybe we can improve the error message?

Agree. The error message confused me.

@hao-lee hao-lee changed the title map: set default pinpath if not provided map: detect map Name and PinPath separately Sep 13, 2021
@hao-lee
Copy link
Contributor Author

hao-lee commented Sep 13, 2021

@lmb Hello, I have updated the commit. The current implementation separates error detections. This should be more reasonable than setting default path.

map.go Outdated
}

if opts.PinPath == "" {
return nil, fmt.Errorf("pin by name: missing PinPath")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe MapOptions.PinPath?

Detecting map Name and PinPath separately to make the error message more clear.

Signed-off-by: Hao Lee <haolee@didiglobal.com>
@lmb lmb merged commit e07f432 into cilium:master Sep 14, 2021
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.

None yet

2 participants