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

Initial inet_ntop implementation #269

Merged
merged 1 commit into from Dec 21, 2018

Conversation

@dalehamel
Copy link
Contributor

commented Nov 26, 2018

This is ready for review.

I have implemented ntop as described in #30, but with a few limitations:

  • Only ipv4 addresses are supported. The main reason ipv6 addresses aren't supported is that there is no way to pass them in right now, is they are stored as a 4x32 bit array in kernel, and there is no way to assign this type to a variable in bpftrace in order to pass it in
  • The constants AF_INET and AF_INET6 would need to be added as builtin idents to bpftrace, in order to specify them as the first argument, as the #define macros from the kernel headers won't be pulled in as any sort of usable variable (and probably shouldn't). We could define them builtin idents pretty easily using a similar approach to nsecs, but for now you must specify the literal 2 for AF_INET, and the literal 10 for (currently unsupported) AF_INET6

The current way to use this function is as so:

./bpftrace -e 'BEGIN{ printf("%s\n", ntop(2, 0xFFFFFFFF)); }'
Attaching 1 probe...
255.255.255.255
  • Add tests for printing ipv4 addresses this sort of functional test would require a new test harness, not sure if worth the effort. I added what tests I could
  • Add map support to be able to use ipv4 addresses in map logic
  • ~Codegen tests should probably be added, but test suit is failing... #187 ~ Added for use as map key, but nothing else.
  • Write llvm IR to copy bytes to userspace buffer properly
@dalehamel

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

Basic working implementation:

 ./bpftrace -e 'BEGIN { printf("%s\n", ntop(2, 0x6c39950a)); }'
Attaching 1 probe...
10.149.57.108

@dalehamel dalehamel changed the title Minimal inet_ntop implementation WIP - Minimal inet_ntop implementation Nov 26, 2018

@dalehamel dalehamel referenced this pull request Nov 26, 2018
3 of 3 tasks complete

@dalehamel dalehamel changed the title WIP - Minimal inet_ntop implementation WIP - Initial inet_ntop implementation Nov 27, 2018

@dalehamel dalehamel force-pushed the dalehamel:inet-ntop branch from c9b0d3d to b80ccb9 Nov 27, 2018

@dalehamel dalehamel changed the title WIP - Initial inet_ntop implementation Initial inet_ntop implementation Nov 27, 2018

@dalehamel

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@ajor when you have a minute, review appreciated :)

@dalehamel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

Sorry for the second ping @ajor, just wondering if I should keep this PR open.

I have a couple of open PRs here and would like some feedback on if this is in line with the vision for bpftrace, or if you've just been busy / swamped and haven't been able to review.

@brendangregg

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

I had a look, looks good. Only two minor comments:

  • this error: "Only AF_INET ipv4 address family is supported by ntop." -- should say "currently", eg, "ntop() currently only supports AF_INET (IPv4); IPv6 will be supported in the future."
  • ntop() should be included in the reference guide.

Plus collapse the commits. Thanks!

@dalehamel dalehamel force-pushed the dalehamel:inet-ntop branch 5 times, most recently from 57619aa to 16bc446 Dec 20, 2018

@dalehamel dalehamel force-pushed the dalehamel:inet-ntop branch from 16bc446 to b470700 Dec 20, 2018

@dalehamel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

I've collapsed the commits, updated the reference guide with usage and a couple of examples, and added the note to the warning message if trying to parse an ipv6 address.

@brendangregg

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

Thanks, looks good to me, I'll merge.

The tests currently have 45 failed tests, but I don't believe this PR caused them, and I'm hoping #292 cleans up these failed tests.

@brendangregg brendangregg merged commit 2ae2a53 into iovisor:master Dec 21, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@dalehamel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

The tests currently have 45 failed tests, but I don't believe this PR caused them

Yes, all PRs are red, but I think it's due to the known issue of the codegen fixtures failing, as I've only added new fixtures here. Thanks for the tip on #292, I'll take a look there 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.