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

Extend type of service/traffic class socket options #2081

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

spencercw
Copy link
Contributor

No description provided.

@asomers
Copy link
Member

asomers commented Aug 6, 2023

This looks good, but would it be possible to add a test?

@spencercw spencercw force-pushed the traffic-class branch 3 times, most recently from 3eee6b8 to 3833746 Compare August 14, 2023 09:36
@spencercw
Copy link
Contributor Author

Added tests and rebased on master. The Linux aarch64 failure seems to be a transient network error.

@SteveLauC
Copy link
Member

Hi, sorry for the late reply.

Something at Nix has changed so that this PR needs an update:

  1. We switched to a new mode for adding changelogs to avoid conflicts, please see CONTRIBUTING.md on how to do it

  2. Now we prefer cfg alias to avoid big cfg chunks, please see CONVENTIONS.md

    For example, the following code can be simplified to:

    #[cfg(any(
        target_os = "android",
        target_os = "freebsd",
        target_os = "ios",
        target_os = "linux",
        target_os = "macos",
        target_os = "netbsd",
        target_os = "openbsd",
    ))]
    
    #[cfg(any(linux_android, netbsdlike, target_os = "freebsd", target_os = "ios", target_os = "macos"))]
    

@spencercw
Copy link
Contributor Author

Thanks for reviewing. Changes applied.

src/sys/socket/mod.rs Outdated Show resolved Hide resolved
),
None => return,
};
let receive = socket(
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to write the test to use socketpair instead of to create two sockets and connect them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied this pattern from nearby tests. Might be better to leave it as is for the sake of consistency?

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.

None yet

3 participants