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

Add bindings for v0.18.0 and backport USDT support. #64

Merged
merged 7 commits into from
Mar 24, 2021

Conversation

tobz
Copy link
Contributor

@tobz tobz commented Mar 21, 2021

This PR is primarily centered around adding USDT support.

We've added bindings for v0.18.0 as I initially did so to test adding USDT support. Once that was sorted, per a comment on this PR, I went and backported USDT support all the way to v0.6.1 by generating new bindings.

Included in the PR, as well, is a simple shell script, suitable for use on Ubuntu, to make generating bindings a little more repeatable.

Closes #62

@tobz tobz marked this pull request as draft March 21, 2021 19:37
@brayniac
Copy link
Contributor

Thanks for looking at this. I don't particularly object to regenerating previous versions where we can add USDT support. I don't think it would break anything that we're already exposing to do that. Adding support for 0.18.0 is definitely welcome though.

@tobz
Copy link
Contributor Author

tobz commented Mar 23, 2021

Yeah, I can take a stab at regenerating older bindings where USDT support was present.

(Note to self: based on cruising the Git tags for iovisor/bcc, we can likely go back as far as v0.6.0 based on the C API methods we need in rust-bpf/rust-bcc#171 for getting USDT support working.)

@tobz tobz changed the title [WIP] Add support for USDT (via v0.18.0) Add bindings for v0.18.0 and backport USDT support. Mar 24, 2021
@tobz tobz marked this pull request as ready for review March 24, 2021 17:52
@tobz
Copy link
Contributor Author

tobz commented Mar 24, 2021

@brayniac I believe this is ready for a proper review now.

  • I've added bindings for v0.18.0.
  • I've backported USDT support from versions v0.6.1 and up.
  • I've added a simple Ubuntu-specific script for helping generate bindings in the future.

All tests pass, and looking at the regenerated bindings by hand seems like it's mostly just clean-up from the bindgen side i.e. better handling of struct alignment and other BC changes. There's a bit of reordering of types and such that make the diffs a little gnarly to inspect. Not sure what the best way would be to better highlight actual changes vs code reordering, but maybe this doesn't matter as long as the tests pass?

@brayniac
Copy link
Contributor

Thanks @tobz - yeah, for this crate I think we have to rely on the smoketest and then making sure it's all working with the bcc crate.

It looks like I've forgotten to modify the smoketest to cover 0.16.0 and 0.17.0 - see https://github.com/rust-bpf/bcc-sys/blob/master/examples/smoketest.rs#L33 - it passed when I merged both of those, but now that they're not the latest versions... we're technically not testing that they didn't obviously break with this PR. If you don't mind fixing that up so we have proper test coverage for this PR that would be great.

Otherwise this looks good. Thanks for spending time on the USDT support!

@tobz
Copy link
Contributor Author

tobz commented Mar 24, 2021

Alright. smoke test adjusted, and things still look good.

@brayniac brayniac merged commit 3cdf88b into rust-bpf:master Mar 24, 2021
@tobz tobz deleted the tobz/v0_18_0_and_usdt_and_docker branch March 24, 2021 19:38
@brayniac
Copy link
Contributor

v0.18.0 is published to crates.io

Thanks again!

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.

bcc USDT bindings
2 participants