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

Add support of TCP_CONGESTION for setsockopt #972

Merged
merged 1 commit into from
Dec 6, 2018

Conversation

Fensteer
Copy link
Contributor

Implementation proposal for support of TCP_CONGESTION param for setsockoptand getsockopt with the CString type.

src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
@Fensteer
Copy link
Contributor Author

I use this code to test the feature but I don't know how to integrate into the CI (I don't know if reno is in tcp_available_congestion_control)

extern crate nix;

use nix::sys::socket::sockopt;
use nix::sys::socket::{getsockopt, setsockopt, socket, AddressFamily, SockFlag, SockType};
use std::ffi::CString;

fn main() {
    let socket = socket(
        AddressFamily::Inet,
        SockType::Stream,
        SockFlag::empty(),
        None,
    ).unwrap();

    let algo = CString::new("reno").expect("Cannot create CString");

    setsockopt(socket, sockopt::TcpCongestion, &algo).expect("setsockopt");

    assert_eq!(algo, CString::new("reno").unwrap());

    assert_eq!(
        getsockopt(socket, sockopt::TcpCongestion).unwrap(),
        CString::new("reno").unwrap()
    );
}

@Fensteer
Copy link
Contributor Author

I don't understand why the buildbot is failing because libc::TCP_CONGESTION exists in latest version of libc : https://docs.rs/libc/0.2.44/libc/constant.TCP_CONGESTION.html

@asomers
Copy link
Member

asomers commented Nov 26, 2018

Looks like libc has only defined TCP_CONGESTION for Linux and uclibc. You'll have to submit a PR to libc to add the definition for FreeBSD. You should also check if it's defined on any other OS.

@Fensteer
Copy link
Contributor Author

Done : rust-lang/libc#1151

@Fensteer
Copy link
Contributor Author

rust-lang/libc#1151 was merged in libc.
What do you think of this version?

src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/sockopt.rs Show resolved Hide resolved
src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
src/sys/socket/sockopt.rs Outdated Show resolved Hide resolved
@asomers
Copy link
Member

asomers commented Dec 3, 2018

Ok, this PR is starting to look pretty good. I think there are only two issues left:

  1. Could you add a test? On FreeBSD, I believe getting the option should work fine. Setting it is a little harder to test, because there's only one valid option unless you load a kernel module. But you should be able to set it back to the same value as the default. I'm not sure what will happen on Linux; you'll have to try to find out.
  2. You'll need to rebase and fix the changelog, since Nix just released 0.12.0.

@Fensteer
Copy link
Contributor Author

Fensteer commented Dec 5, 2018

CI fails but actually the code is OK. I tried CI on a full emulated MIPS or PowerPC system and the test run well. I think that the CI fails because only the binary is run on a emulated processor but the kernel is still the same than the host.

@asomers
Copy link
Member

asomers commented Dec 5, 2018

CI fails but actually the code is OK. I tried CI on a full emulated MIPS or PowerPC system and the test run well. I think that the CI fails because only the binary is run on a emulated processor but the kernel is still the same than the host.

QEMU does emulate the full system. It's not just a binary translator. However, it's got a lot of bugs. What did you use for a "full emulated MIPS or PowerPC system"?

test/sys/test_sockopt.rs Outdated Show resolved Hide resolved
@Fensteer Fensteer force-pushed the tcp_congestion branch 2 times, most recently from 22cb17d to 7fa3cfb Compare December 6, 2018 08:40
@Fensteer
Copy link
Contributor Author

Fensteer commented Dec 6, 2018

Rebase done and I squashed some commits.
CHANGELOG fixed.

@asomers
Copy link
Member

asomers commented Dec 6, 2018

Could you please squash all commits together?

2 similar comments
@asomers
Copy link
Member

asomers commented Dec 6, 2018

Could you please squash all commits together?

@asomers
Copy link
Member

asomers commented Dec 6, 2018

Could you please squash all commits together?

@asomers
Copy link
Member

asomers commented Dec 6, 2018

Could you please squash all commits together?

@Fensteer
Copy link
Contributor Author

Fensteer commented Dec 6, 2018

Done

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Sorry for the comment dupes; github seems to have a partial outage today.

bors r+

bors bot added a commit that referenced this pull request Dec 6, 2018
972: Add support of TCP_CONGESTION for setsockopt r=asomers a=Fensteer

Implementation proposal for support of TCP_CONGESTION param for `setsockopt`and `getsockopt` with the CString type.

Co-authored-by: Fensteer <fensteer@protonmail.com>
@bors
Copy link
Contributor

bors bot commented Dec 6, 2018

Build succeeded

@bors bors bot merged commit b75d31d into nix-rust:master Dec 6, 2018
@Fensteer
Copy link
Contributor Author

Fensteer commented Dec 6, 2018

Thanks for the reviews and advices !

@Fensteer Fensteer deleted the tcp_congestion branch December 6, 2018 16:54
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.

2 participants