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

Refactor & clean code #93

Closed
wants to merge 174 commits into from
Closed

Refactor & clean code #93

wants to merge 174 commits into from

Conversation

ssrlive
Copy link
Collaborator

@ssrlive ssrlive commented Sep 2, 2024

No description provided.

dependabot bot and others added 27 commits July 23, 2024 11:16
Extending automatic temporary lifetime extension
Updates the requirements on [windows-sys](https://github.com/microsoft/windows-rs) to permit the latest version.
- [Release notes](https://github.com/microsoft/windows-rs/releases)
- [Commits](microsoft/windows-rs@0.52.0...0.59.0)

---
updated-dependencies:
- dependency-name: windows-sys
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
chore(deps): update windows-sys requirement from 0.52 to 0.59
Impl Deref/DerefMut traits for AsyncDevice
* compatible with low-version rustc

* pass clippy

* Bump version
@ssrlive ssrlive closed this Sep 2, 2024
@ssrlive ssrlive deleted the clear branch September 2, 2024 14:14
Copy link

@hyrumcoop hyrumcoop left a comment

Choose a reason for hiding this comment

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

I've left some comments pointing out some features that have been removed/deprecated without proper explanation.

I was initially excited that this project was reactivated by new maintainers, but it's discouraging to see so much of the functionality this library provides wiped out in one massive PR. Hoping to see some of these features re-incorporated.

@@ -116,6 +146,8 @@ impl Configuration {
}

/// Set the number of queues.
/// Note: The queues must be 1, otherwise will failed.
#[deprecated(since = "1.0.0", note = "The queues will always be 1.")]

Choose a reason for hiding this comment

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

Is there a new API for tun multi-queueing? Why deprecate this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For consistency reasons, it is difficult to provide multiple queue features on platforms other than Linux.
If you would like to contribute, PRs are welcome.

/// each packet delivered from/to Linux underlying API is a header with flags and protocol type when enabled.
///
/// [Note: This configuration just applies to the Linux underlying API and is a no-op on tun2(i.e. the packets delivered from/to tun2 always contain no packet information) -- end note].
#[deprecated(

Choose a reason for hiding this comment

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

Why is this deprecated? Users may wish to turn on packet information, regardless of what the default is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The packet_information switch is active and will affect the format of data packets generated by Linux TUN.

But the tun crate will determine whether it contains packet_information.
If so, the tun crate will remove or add back it; if not, it will do nothing.

}

fn read_vectored(&mut self, bufs: &mut [io::IoSliceMut<'_>]) -> io::Result<usize> {

Choose a reason for hiding this comment

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

Why did you eliminate the read_vectored implementation with the underlying readv syscall? This is a distinct syscall from read that uses scatter-gather I/O. This breaks caller expectations and may have a large perf impact in some usages.

Copy link
Collaborator Author

@ssrlive ssrlive Nov 19, 2024

Choose a reason for hiding this comment

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

Um... you can decide whether to add it back or not.

Ok(())
}

fn write_vectored(&mut self, bufs: &[io::IoSlice<'_>]) -> io::Result<usize> {

Choose a reason for hiding this comment

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

See my comment about read_vectored and readv.

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.