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

Improve-ephemeral-peer-error-logs #7396

Merged
merged 7 commits into from
Dec 20, 2024
Merged

Conversation

Serock3
Copy link
Contributor

@Serock3 Serock3 commented Dec 20, 2024


This change is Reviewable

@Serock3 Serock3 self-assigned this Dec 20, 2024
@Serock3 Serock3 force-pushed the improve-ephemeral-peer-error-logs branch from ec1149a to ed10ac2 Compare December 20, 2024 15:47
@Serock3 Serock3 requested a review from hulthe December 20, 2024 15:47
dlon
dlon previously approved these changes Dec 20, 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.

:shipit:

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

Copy link
Contributor

@hulthe hulthe 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: all files reviewed, 1 unresolved discussion (waiting on @Serock3)


talpid-wireguard/src/lib.rs line 978 at r1 (raw file):

async fn log_tunnel_data_usage(config: &Config, tunnel: &Arc<AsyncMutex<Option<Box<dyn Tunnel>>>>) {
    let tunnel = tunnel.lock().await;
    let Ok(tunnel_stats) = tunnel.as_ref().unwrap().get_tunnel_stats() else {

nit: maybe prefer to just return if the tunnel is None instead of unwrapping, even if it currently will always be Some

Copy link
Contributor

@hulthe hulthe 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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @dlon and @Serock3)


talpid-wireguard/src/lib.rs line 978 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

nit: maybe prefer to just return if the tunnel is None instead of unwrapping, even if it currently will always be Some

i.e.

    let Some(tunnel) = &*tunnel else { return }

Copy link
Contributor

@hulthe hulthe 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: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @dlon and @Serock3)


talpid-tunnel-config-client/src/lib.rs line 361 at r1 (raw file):

    ) -> std::task::Poll<std::io::Result<()>> {
        std::pin::Pin::new(&mut self.s).poll_shutdown(cx)
    }

nit : There's a lot of fully qualified type paths here, a bit too much noise imo. Maybe consider removing some of them? Also, isn't Unpin in the prelude??

Copy link
Contributor

@hulthe hulthe 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: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @dlon and @Serock3)


talpid-tunnel-config-client/src/lib.rs line 330 at r1 (raw file):

        let bytes = std::task::ready!(std::pin::Pin::new(&mut self.s).poll_read(cx, buf));
        if bytes.is_ok() {
            self.rx_bytes += buf.filled().len();

Nothing's stopping the caller from providing a partially filled ReadBuf here, in which case this addition would be wrong.

Copy link
Contributor

@hulthe hulthe 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: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @dlon and @Serock3)


talpid-tunnel-config-client/src/lib.rs line 344 at r1 (raw file):

        let bytes = std::task::ready!(std::pin::Pin::new(&mut self.s).poll_write(cx, buf));
        if bytes.is_ok() {
            self.tx_bytes += buf.len();

The inner writer is not guaranteed to write all of buf. You need to inspect bytes (which is a Result<usize>) to see how many bytes were written.

Copy link
Contributor Author

@Serock3 Serock3 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: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @dlon and @hulthe)


talpid-tunnel-config-client/src/lib.rs line 330 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Nothing's stopping the caller from providing a partially filled ReadBuf here, in which case this addition would be wrong.

Done.


talpid-tunnel-config-client/src/lib.rs line 344 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

The inner writer is not guaranteed to write all of buf. You need to inspect bytes (which is a Result<usize>) to see how many bytes were written.

Done.

hulthe
hulthe previously approved these changes Dec 20, 2024
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Serock3 Serock3 force-pushed the improve-ephemeral-peer-error-logs branch from d2474c1 to 92eb9f4 Compare December 20, 2024 16:32
@Serock3 Serock3 force-pushed the improve-ephemeral-peer-error-logs branch from 92eb9f4 to 05e01e9 Compare December 20, 2024 16:37
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 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Serock3 Serock3 merged commit 9b3a32b into main Dec 20, 2024
56 checks passed
@Serock3 Serock3 deleted the improve-ephemeral-peer-error-logs branch December 20, 2024 16:47
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.

3 participants