-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implement function cgroupid(path) -> cgroup id #170
Implement function cgroupid(path) -> cgroup id #170
Conversation
I also tried to build from a Fedora Docker image, without success (build log). |
Yes, we think there's a different issue with Docker builds at the moment. This patch looks good. |
Just noticed: this needs an addition to the reference guide as well. Thanks! |
The Fedora Docker build errors are because it’s using LLVM 7 - see #149 The Docker errors we’ve got at the moment are limited to string comparisons so I’d rather not completely break the Alpine build |
src/bpftrace.cpp
Outdated
uint64_t cgid; | ||
int err, mount_id; | ||
file_handle *handle = reinterpret_cast<struct file_handle *>( | ||
malloc(sizeof(file_handle) + sizeof(uint64_t))); |
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.
Can handle just be allocated on the stack instead here?
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.
Oh, I see it needs more space than the struct now...
Could we do something like create our own wrapper struct with the regular file_handle and a uint64_t inside to avoid the dynamic allocation then? Or use smart pointers - I just don't like having malloc and free in here
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.
ok, I'll rewrite it to put it on the stack.
Maybe we could just #ifdef this code out so it's not available from Alpine/Docker builds until the next release of musl |
src/ast/semantic_analyser.cpp
Outdated
@@ -254,6 +254,14 @@ void SemanticAnalyser::visit(Call &call) | |||
} | |||
call.type = SizedType(Type::integer, 8); | |||
} | |||
else if (call.func == "cgroupid") { | |||
if (check_nargs(call, 1)) { | |||
if (check_arg(call, Type::string, 0, true)) { |
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.
This if
is pointless, no? So just having check_arg(call, …);
should be enough.
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.
I was reproducing the coding style from above
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.
Appreciate you sticking to the file's coding style, but I think in this case that code is just wrong
src/bpftrace.cpp
Outdated
{ | ||
uint64_t cgid; | ||
int err, mount_id; | ||
file_handle *handle = reinterpret_cast<struct file_handle *>( |
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 struct
keyword inside reinterpret cast
is unnecessary.
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.
ok, I'll remove it.
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.
I had a look at the alpine:edge
and alpine:latest
docker images but they still don't provide name_to_handle_at
. I'll add #ifdef
for now, as suggested.
src/ast/semantic_analyser.cpp
Outdated
@@ -254,6 +254,14 @@ void SemanticAnalyser::visit(Call &call) | |||
} | |||
call.type = SizedType(Type::integer, 8); | |||
} | |||
else if (call.func == "cgroupid") { | |||
if (check_nargs(call, 1)) { | |||
if (check_arg(call, Type::string, 0, true)) { |
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.
I was reproducing the coding style from above
src/bpftrace.cpp
Outdated
{ | ||
uint64_t cgid; | ||
int err, mount_id; | ||
file_handle *handle = reinterpret_cast<struct file_handle *>( |
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.
ok, I'll remove it.
src/bpftrace.cpp
Outdated
uint64_t cgid; | ||
int err, mount_id; | ||
file_handle *handle = reinterpret_cast<struct file_handle *>( | ||
malloc(sizeof(file_handle) + sizeof(uint64_t))); |
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.
ok, I'll rewrite it to put it on the stack.
4ef61ef
to
86f6ca4
Compare
Updated the PR, I think I addresses all the review issues. But running
|
Uh, I still need to add it to the reference guide, I haven't noticed it before. |
I doubt your changes broke build.sh, that's likely a separate issue we need to fix. I've been doing manual builds on ubuntu as shown in install.md. I'd suggest improving this error message:
Instead of This also core dumps (it doesn't need to), but that's something we can clean up separately along with the other normal error paths that use throw when they probably should use something else. So right now I'd just change the text. |
build.sh still works for me when I check out your branch. Not sure what's causing the problem you're seeing, but I don't think these changes are responsible |
We will wrap the file_handle struct in the next commit. The struct has a flexible array member, which is not supported by C++. Compiler may complain about using it when allocated on stack, even indirectly as a member of a struct. I'm not sure if using this kind of types is even a defined behavior…
Currently used image of alpine has a version of musl libc that does not have name_to_handle_at function that is required to resolve the cgroup id. In such case we just use an implementation that says that resolving cgroupid is not supported.
Mention the `cgroup` variable and the `cgroupid` function alongside an example that uses them both.
86f6ca4
to
5db00d7
Compare
Updated the PR - changed the error message (I think it would be nice to consume errno there too), updated the reference to mention both |
thanks! |
This PR is the second part to implement #150.
Note: it works fine on my Fedora laptop when compiled with:
But the Docker build stopped building. The alpine builder image is based on musl libc instead of glibc and it does not support
name_to_handle_at
. I tried to switch back to the ubuntu builder image but I got the same error as in this email from linux.debian.bugs.dist and I couldn't fix it.I added a test in tests/codegen.cpp for the builtin
cgroup
but I am not sure how to check the generated instructions. I assume it is fine because it works when I test manually.I didn't add a test for the function
cgroupid()
because it does not generate any interesting BPF instructions but just callsname_to_handle_at()
in userspace. The result of that is depending on the local system. With systemd configured in cgroup hybrid mode, cgroup-v2 would be mounted in/sys/fs/cgroup/unified
but it could be mounted differently in other systemd configuration.