-
Notifications
You must be signed in to change notification settings - Fork 248
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
Canonicalize pin paths #4274
base: main
Are you sure you want to change the base?
Canonicalize pin paths #4274
Conversation
scripts/check_binary_dependencies_ebpfapi_dll_regular_debug.txt
Outdated
Show resolved
Hide resolved
888ff28
to
8717508
Compare
tests/end_to_end/end_to_end.cpp
Outdated
REQUIRE(map_fd > 0); | ||
|
||
// Verify pin path canonicalization. | ||
_verify_canonical_path(map_fd, "\\ebpf\\1", "C:\\ebpf\\1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C:
prefix is kind of unexpected, is there a way to avoid that? The reason is that in Go the canonical way to handle paths is the filepath
package. So to derive the pin path of an object we do:
filepath.Join("/sys/fs/bpf", "./../test")
The result of that function is \sys\fs\test
. I guess this would work fine as long as we never iterate over pins, but it'd be nicer if the common use of the language native API would match up with the canonicalisation. Another way of saying this is: in the absence of a real BPF filesystem on Windows I think it'd be better to do purely lexical processing of the path.
Another question: what influences the C:
in the output filename? Does it change based on the working directory of the calling process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a quick search, https://en.cppreference.com/w/cpp/filesystem/path/lexically_normal or similar might work? Lexical processing doesn't answer the question what should happen to relative paths like foo/bar
, but maybe the right answer is to only accept absolute paths for pinning right now? (So ones starting in \
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
C:
prefix is kind of unexpected, is there a way to avoid that? The reason is that in Go the canonical way to handle paths is thefilepath
package.
Let's discuss more in the meeting. The ":" prefix is the common Windows filesystem mechanism. How does the filepath
package deal with normal filesystem paths then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://pkg.go.dev/path/filepath says that drive letters are supported on Windows, and so it should not be unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to "BPF:" per analysis in the PinPaths.md doc that is now part of this PR.
e45e561
to
53c55e1
Compare
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
81b2d93
to
a20d9c8
Compare
* Pins can be enumerated or removed via normal filesystem APIs/utilities | ||
* Pin paths are case-sensitive | ||
* The path component separator is a forward slash ('/') | ||
* Pin paths begin with "/sys/fs/bpf/" and relative paths like "mymap" are relative to that prefix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily, this is just a convention that systemd and the popular user space libraries follow. It is possible to mount a separate BPF fs at say /var/run/myapp/bpffs and then pin into that.
the driver implements it as such. | ||
|
||
As for portability, canonicalization could ensure that Linux style paths like | ||
"/sys/fs/bpf/my/pin/path" would be canonicalized to "BPF:\my\pin\path" allowing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut feeling would be to not remove the sys/fs/bpf
prefix. If you do want to remove it, what should happen to paths that do not have it?
|
||
As for portability, canonicalization could ensure that Linux style paths like | ||
"/sys/fs/bpf/my/pin/path" would be canonicalized to "BPF:\my\pin\path" allowing | ||
portability in that respect. But querying the pin path on an object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete sentence?
|
||
1. RemoteFile: Avoid confusion with remote paths. | ||
|
||
1. FileAPIs: Paths should work with Windows file APIs in the future once a BPF filesystem is implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a necessary requirement? I had a very brief search on how this would be achieved on Windows and my impression is that a filesystem is a significant piece of work?
At this point, we already have support for calling into the user space APIs in C and Go. I believe most other libraries will provide an API for unpinning (rather than expecting the user to do straight up file system calls). Do we gain much by switching to a real filesystem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. If we want to keep the filesystem route open we probably need to take https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions into account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether we do it or not is really up to future demand and priorities, but the point is to not preclude it if we do need it in the future.
|
||
### UNC (UNC path) | ||
|
||
Example: \\BPF\my\pin\path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also \\?\
which would disable all path parsing and would forward the path to the filesystem verbatim. And \\.\
which points into the object namespace, but I don't know whether we could hook that somehow?
|
||
### Virtual (Windows file system path with another virtual drive for BPF) | ||
|
||
Example: BPF:\my\pin\path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being a windows noob, is it possible to have multi letter drives? Never seen one of those.
Also, would this drive show up in the explorer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a drive, but a virtual device (aka DOS device name). There are a bunch of these in windows, like CON:, PRN:, NUL:, COM1:, etc. And drivers can add new ones.
Description
ebpf_get_next_pinned_object_path()
where the start_path cannot be null.Fixes #4239
Note: on Linux, pin paths are case sensitive, and so any programs ported from Linux may assume case sensitivity.
On Windows, it varies by filesystem, where Windows itself supports both case sensitive filesystems and case insensitive filesystems. The latter is the normal filesystem on Windows, but if we later support a BPF filesystem, we can make it be case sensitive, for ease of porting from Linux. As such, this PR uses case sensitive pin paths. This is not a change for ebpf-for-windows, which already had case-sensitive pin paths.
Testing
This PR contains associated tests.
Documentation
This PR includes a design doc added to the docs directory.
Installation
No impact.