Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions src/port_forwarding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ use std::ffi::OsStr;

use std::borrow::Cow;
use std::fmt;
use std::io;
use std::net::{self, SocketAddr, ToSocketAddrs};
use std::net::{self, SocketAddr};
use std::path::{Path, PathBuf};

/// Type of forwarding
Expand Down Expand Up @@ -44,12 +43,20 @@ pub enum Socket<'a> {
},

/// Tcp socket.
TcpSocket(SocketAddr),
TcpSocket {
/// Hostname.
host: Cow<'a, str>,
/// Port.
port: u16,
},
}

impl From<SocketAddr> for Socket<'static> {
fn from(addr: SocketAddr) -> Self {
Socket::TcpSocket(addr)
Socket::TcpSocket {
host: addr.ip().to_string().into(),
port: addr.port(),
}
}
}

Expand Down Expand Up @@ -99,19 +106,22 @@ impl From<Box<Path>> for Socket<'static> {

impl Socket<'_> {
/// Create a new TcpSocket
pub fn new<T: ToSocketAddrs>(addr: &T) -> Result<Socket<'static>, io::Error> {
addr.to_socket_addrs()?
.next()
.ok_or_else(|| io::Error::new(io::ErrorKind::Other, "no more socket addresses to try"))
.map(Socket::TcpSocket)
pub fn new<'a, S>(host: S, port: u16) -> Socket<'a>
where
S: Into<Cow<'a, str>>,
Comment on lines +109 to +111
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes the public api, and so would end up requiring a major version bump. Would you mind making this a separate constructor instead?

cc @NobodyXu we have to be a little careful about these kinds of changes. Maybe we should add a CI step that invokes https://github.com/obi1kenobi/cargo-semver-checks or https://github.com/Enselic/cargo-public-api to catch these things automatically (at least in most cases).

Copy link
Member

Choose a reason for hiding this comment

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

This changes the public api, and so would end up requiring a major version bump. Would you mind making this a separate constructor instead?

Sure, but with #131 we probably need a breaking change anyway.

cc @NobodyXu we have to be a little careful about these kinds of changes. Maybe we should add a CI step that invokes https://github.com/obi1kenobi/cargo-semver-checks or https://github.com/Enselic/cargo-public-api to catch these things automatically (at least in most cases).

Yeah, we definitely need semver-checking in CI.

Choose a reason for hiding this comment

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

I'm sorry to chime in, I saw the stream and think there's common mistake regarding SemVer and the zero major version you might not be aware of (and I learned recently as well). :-)

My comment is just for info, choose whatever version you want. I just want to give some hint.

This changes the public api, and so would end up requiring a major version bump.

Technically, there's no need to bump the major version, because 1.0.0 hasn't been released yet. Therefore, everything so far is "initial development".

The following is cited from SemVer spec:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@jonhoo jonhoo Aug 14, 2023

Choose a reason for hiding this comment

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

@stephan-cr in Rust, when the leftmost version number component is a 0, then the next digit over is effectively considered the major version. That's what I was referring to bumping. Updating x in 0.x has the same effect and implications as bumping x in x.0, and so it is still considered (and referred to) as a major version bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a user here, so discount my opinion appropriately, but strong preference from me for a semver-breaking release with just the new API, rather than a backcompatible release that includes both APIs! The existing API is, AFAICT, not possible to use correctly with remote port forwarding, and so forcing users into the new API seems worth the breakage to me.

(Earlier discussion on this: #120 (comment))

{
Socket::TcpSocket {
host: host.into(),
port,
}
}

#[cfg(feature = "process-mux")]
pub(crate) fn as_os_str(&self) -> Cow<'_, OsStr> {
match self {
#[cfg(unix)]
Socket::UnixSocket { path } => Cow::Borrowed(path.as_os_str()),
Socket::TcpSocket(socket) => Cow::Owned(format!("{}", socket).into()),
Socket::TcpSocket { host, port } => Cow::Owned(format!("{host}:{port}").into()),
}
}
}
Expand All @@ -124,9 +134,9 @@ impl<'a> From<Socket<'a>> for native_mux_impl::Socket<'a> {
match socket {
#[cfg(unix)]
Socket::UnixSocket { path } => UnixSocket { path },
Socket::TcpSocket(socket) => TcpSocket {
port: socket.port() as u32,
host: socket.ip().to_string().into(),
Socket::TcpSocket { host, port } => TcpSocket {
host,
port: port as u32,
},
}
}
Expand All @@ -137,9 +147,9 @@ impl<'a> fmt::Display for Socket<'a> {
match self {
#[cfg(unix)]
Socket::UnixSocket { path } => {
write!(f, "{}", path.to_string_lossy())
write!(f, "{}", path.display())
}
Socket::TcpSocket(socket) => write!(f, "{}", socket),
Socket::TcpSocket { host, port } => write!(f, "{host}:{port}"),
}
}
}