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

utils: unpack kheaders.tar.xz if necessary #768

Merged
merged 3 commits into from
Sep 26, 2019
Merged

Conversation

mokomull
Copy link
Contributor

When CONFIG_IKHEADERS is enabled, a tarball containing the kernel
headers is available in /sys/kernel, allowing the kernel type
definitions to be used even if the headers have not been separately
distributed to /lib on the target.

This is akin to how bcc handles it, but with the new path in /sys
after 2. Notably, use the same path in /tmp, so that the tarball will
be unpacked only once between the two tools.

When CONFIG_IKHEADERS is enabled, a tarball containing the kernel
headers is available in /sys/kernel, allowing the kernel type
definitions to be used even if the headers have not been separately
distributed to /lib on the target.

This is akin to [how bcc handles it][1], but with the new path in /sys
after [2].  Notably, use the same path in /tmp, so that the tarball will
be unpacked only once between the two tools.

[1]: iovisor/bcc@ae92f3d
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f7b101d33046a837c2aa4526cef28a3c785d7af2
@joelagnel
Copy link

Thanks for doing this. Could you read the /tmp directory path from $TMPDIR if it exists? If it doesn't then default to /tmp.

@danobi
Copy link
Member

danobi commented Jul 11, 2019

Sounds reasonable.

@mokomull
Copy link
Contributor Author

mokomull commented Aug 2, 2019

I added the functionality to use getenv("TMPDIR") if present; it seems to work as expected:

Without $TMPDIR set, this does:

2692  execve("./src/bpftrace", ["./src/bpftrace", "--include", "linux/blkdev.h", "-e", "tracepoint:nbd:nbd_send_request "...], 0x7ffed5225e68 /* 12 vars */) = 0
2693  execve("/bin/sh", ["sh", "-c", "tar xf /sys/kernel/kheaders.tar."...], 0x7ffd38230288 /* 12 vars */) = 0
2693  execve("/usr/bin/tar", ["tar", "xf", "/sys/kernel/kheaders.tar.xz", "-C", "/tmp/kheaders-nyIVgE"], 0x55e096604c50 /* 15 vars */) = 0
2694  execve("/usr/local/sbin/xz", ["xz", "-d"], 0x7fffd762f4b8 /* 15 vars */) = -1 ENOENT (No such file or directory)
2694  execve("/usr/local/bin/xz", ["xz", "-d"], 0x7fffd762f4b8 /* 15 vars */) = -1 ENOENT (No such file or directory)
2694  execve("/usr/bin/xz", ["xz", "-d"], 0x7fffd762f4b8 /* 15 vars */) = 0
[...]
2692  rename("/tmp/kheaders-nyIVgE", "/tmp/kheaders-5.3.0-rc2+") = 0

and with $TMPDIR set:

2700  execve("./src/bpftrace", ["./src/bpftrace", "--include", "linux/blkdev.h", "-e", "tracepoint:nbd:nbd_send_request "...], 0x7ffeee7b8038 /* 13 vars */) = 0
2701  execve("/bin/sh", ["sh", "-c", "tar xf /sys/kernel/kheaders.tar."...], 0x7ffc903ce4e8 /* 13 vars */) = 0
2701  execve("/usr/bin/tar", ["tar", "xf", "/sys/kernel/kheaders.tar.xz", "-C", "/tmp/hi/kheaders-Kj6Lu4"], 0x56401da74630 /* 16 vars */) = 0
2702  execve("/usr/local/sbin/xz", ["xz", "-d"], 0x7ffec6e757a8 /* 16 vars */) = -1 ENOENT (No such file or directory)
2702  execve("/usr/local/bin/xz", ["xz", "-d"], 0x7ffec6e757a8 /* 16 vars */) = -1 ENOENT (No such file or directory)
2702  execve("/usr/bin/xz", ["xz", "-d"], 0x7ffec6e757a8 /* 16 vars */) = 0
[...]
2700  rename("/tmp/hi/kheaders-Kj6Lu4", "/tmp/hi/kheaders-5.3.0-rc2+") = 0

The behavior when $TMPDIR is set, but is not a directory, could possibly be improved; I'm open to feedback on this:

[mmullins@arch build]$ touch /tmp/bye
[mmullins@arch build]$ sudo TMPDIR=/tmp/bye ./src/bpftrace --include linux/blkdev.h -e 'tracepoint:nbd:nbd_send_request { printf("%d", args->request->cmd_flags); }'
terminate called after throwing an instance of 'std::runtime_error'
  what():  creating temporary path for kheaders.tar.xz failed
Aborted

This codepath is only hit if none of /lib/modules/$(uname -r)/{source,build} are present, though, so there's no regression for currently-working scenarios that I can imagine.

Copy link
Member

@danobi danobi left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, couple nits

src/utils.cpp Outdated

KernelHeaderTmpDir tmpdir{path_prefix};

FILE* tar = popen(("tar xf /sys/kernel/kheaders.tar.xz -C " + tmpdir.path).c_str(), "w");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FILE* tar = popen(("tar xf /sys/kernel/kheaders.tar.xz -C " + tmpdir.path).c_str(), "w");
FILE* tar = ::popen(("tar xf /sys/kernel/kheaders.tar.xz -C " + tmpdir.path).c_str(), "w");

src/utils.cpp Outdated
return "";
}

int rc = pclose(tar);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int rc = pclose(tar);
int rc = ::pclose(tar);

src/utils.cpp Outdated

std::string path;

friend std::string unpack_kheaders_tar_xz(const struct utsname&);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to instead make everything public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wondered that one myself when I went back to post v2 yesterday :) That should shave off a few lines of weirdness.

Copy link
Member

@danobi danobi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@danobi danobi merged commit 896fafb into bpftrace:master Sep 26, 2019
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.

3 participants