search

kentik / kprobe

GNU General Public License v2.0
15 stars 4 forks source link

First stab at adding a region flag #1

Closed i3149 closed 5 years ago

i3149 commented 5 years ago

Adding a region flag to be in line with kprobe. I don't think I did this right, but man I hit a lot of rust wierdness here. Anyrate it compiles. Interested to see what you do with this, but low priority.

i3149 commented 5 years ago

Well, fails because wrong nightly code is running. Worth it to pin to a known working in the build.sh?

wg commented 5 years ago

Hmm yeah looks like the kt-build-rust image was rebuilt for some reason and pulled a newer nightly. The correct way is to make a rust-toolchain file, there's a branch with that but it didn't build on Jenkins and didn't feel like spending a lot of time tracking it down ;-) Need to get the latest nightlies working anyway, so maybe better to spend the time doing that.

wg commented 5 years ago

Using as_ptr() can be tricky because the Rust compiler can't track when the pointee is deallocated and the pointer becomes invalid. In this case the CString in CString::new("...").unwrap().as_ptr() is deallocated (dropped in Rust terminology) immediately and so the pointer is invalid. Here's a snippet demonstrating it: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4b45cd8303969524ec1254c45d65182b

The other uses of as_ptr() are safe because they all point into parts of the &Config which is passed in as an argument, and so lives longer than the call to libkflow::configure(cfg: &Config). And of course libkflow's kflowInit(...) method is careful to use C.GoString() to copy all the strings it needs to reference later.

wg commented 5 years ago

Ok I've done this with a slightly different approach =) https://github.com/kentik/kprobe/commit/453825f4f229699ede8d92791e5e8b67843764d0

This way you can specify --region but still override one or more of the URLs if needed.

Also fixed the issue building with recent nightlies, think something changed and it wasn't static linking by default anymore.