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

Bump pcap to address TODO comment in split tunnel implementation #6685

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Aug 27, 2024

This PR replaces some auto-generated libpcap bindings for macOS with recently (WIP) released APIs in the pcap crate. By using Capture::want_pktap we can get rid of 3 uses of unsafe in the implementation of split tunneling on macOS, while removing some no-longer needed bindings.


This change is Reviewable

@MarkusPettersson98 MarkusPettersson98 added dependencies Pull requests that update a dependency file macOS Issues related to macOS labels Aug 27, 2024
Copy link

linear bot commented Aug 27, 2024

@MarkusPettersson98 MarkusPettersson98 force-pushed the cleanup-libpcap-bindings-for-macos-des-1188 branch 4 times, most recently from 7003ab9 to cad48fc Compare August 27, 2024 12:56
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review August 27, 2024 12:57
@MarkusPettersson98 MarkusPettersson98 force-pushed the cleanup-libpcap-bindings-for-macos-des-1188 branch from cad48fc to c6a8ceb Compare August 27, 2024 12:59
@MarkusPettersson98 MarkusPettersson98 changed the title WIP Bump pcap to address TODO comment in split tunnel implementation Bump pcap to address TODO comment in split tunnel implementation Aug 27, 2024
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Great cleanup!

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)


talpid-core/src/split_tunnel/macos/bindings.rs line 1 at r1 (raw file):

// automatically generated by rust-bindgen 0.69.2

Note that this file is generated by a bash script/header. So it should be updated.


talpid-core/src/split_tunnel/macos/tun.rs line 810 at r1 (raw file):

    // libpcap will do the heavy lifting for us if we simply request a "pktap" device.
    //
    // TODO: upstream exposure of a raw handle to pcap_t on Capture<Inactive>

I think we don't need the raw pointer, so we can probably remove this TODO comment as well.

@MarkusPettersson98 MarkusPettersson98 force-pushed the cleanup-libpcap-bindings-for-macos-des-1188 branch from c6a8ceb to 6d98b68 Compare August 27, 2024 15:44
Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @dlon)


talpid-core/src/split_tunnel/macos/bindings.rs line 1 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Note that this file is generated by a bash script. So it should be updated.

Oh, of course! So satisfying 👍


talpid-core/src/split_tunnel/macos/tun.rs line 810 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think we don't need the raw pointer, so we can probably remove this TODO comment as well.

Done

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Use the safe `pcap` function `Direction::want_pktap` to initialize the
packet capture in the macOS split tunnel implemenation. This replaces
parts of our auto-generated FFI to interface with `libpcap`, thus
getting rid of some use of `unsafe`.
@MarkusPettersson98 MarkusPettersson98 force-pushed the cleanup-libpcap-bindings-for-macos-des-1188 branch from 5e77045 to 43b276e Compare August 28, 2024 07:19
@MarkusPettersson98 MarkusPettersson98 merged commit 7ead1cc into main Aug 28, 2024
54 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the cleanup-libpcap-bindings-for-macos-des-1188 branch August 28, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file macOS Issues related to macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants