Skip to content

TcpStream (and probably more std::net types) missing Debug impl #23134

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.

Sign up for GitHub

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

Jump to bottom
Closed
carllerche opened this issue Mar 6, 2015 · 10 comments · Fixed by #25078
Closed

TcpStream (and probably more std::net types) missing Debug impl #23134

carllerche opened this issue Mar 6, 2015 · 10 comments · Fixed by #25078
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@carllerche
Copy link
Member

It seems like these types could implement Debug (allowing structs that contain std::net values to be able to derive Debug).

cc @alexcrichton
@sfackler sfackler self-assigned this Mar 7, 2015
@kmcallister kmcallister added A-libs E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Mar 7, 2015
@wesleywiser
Copy link
Member

Hi, if it's ok, I'd like to work on this issue. I have a commit in my local repo which resolves this but I need to test it some more to be sure. Hopefully, I can open a PR with the changes later tonight.

@sfackler
Copy link
Member

sfackler commented Mar 9, 2015

Sure!

@sfackler sfackler removed their assignment Mar 9, 2015
@wesleywiser
Copy link
Member

@sfackler Hi, I'm trying to test my changes but I'm running into an issue. make help says that running make all should result in rustc being in ${platform}/stage2/bin but when I run ./rustc from that directory, I get:

./rustc: symbol lookup error: ./rustc: undefined symbol: _ZN2rt10lang_start20hd326254c68b07ef5NFJE

Any ideas?

@sfackler
Copy link
Member

You need to set your LD_LIBRARY_PATH on linux or DYLD_LIBRARY_PATH on osx to have it point to the right set of libraries - it should be set to ${platform}/stage2/lib.

@wesleywiser
Copy link
Member

Thanks, that did the trick.

I found several types in std::net that didn't implement Debug. After my changes this is what they output:

  • Shutdown: Read or Write or Both
  • TcpStream: TcpStream { addr: Ok(127.0.0.1:1234) }
  • TcpListener: TcpListener { addr: Ok(127.0.0.1:1234) }
  • UdpSocket: UdpSocket { addr: Ok(127.0.0.1:1234) }

Does that look right? Should I try to get rid of the inner Oks?

@wesleywiser
Copy link
Member

@sfackler Sorry, I think I forgot to mention you in that last comment.

@sfackler
Copy link
Member

Ah yeah, sorry. I'd go without the Ok wrappers personally. It's probably also a good idea to put the file descriptor itself in the debug message as well.

wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 29, 2015
@wesleywiser
Implement Debug for some types in std::net
5658ac9 
Implement types for:
- Shutdown
- TcpStream
- TcpListener
- UdpSocket

Fixes rust-lang#23134
@nham
Copy link
Contributor

nham commented Apr 24, 2015

@wesleywiser Are you still working on this?

@wesleywiser
Copy link
Member

No, I ran into some issues trying to figure out how to get at the file
descriptors.

On Fri, Apr 24, 2015, 4:31 PM Nick Hamann [email protected] wrote:

@wesleywiser https://github.com/wesleywiser Are you still working on
this?

—
Reply to this email directly or view it on GitHub
#23134 (comment).

@nham
Copy link
Contributor

nham commented May 3, 2015

I've started working on this. I have a branch here: nham@6ac730a

nham pushed a commit to nham/rust that referenced this issue May 3, 2015
Implement Debug for std::net::{UdpSocket,TcpStream,TcpListener,Shutdown}
3a4a1e5 
Fixes rust-lang#23134.
@nham nham mentioned this issue May 3, 2015
Merged
bors added a commit that referenced this issue May 4, 2015
@bors
Auto merge of #25078 - nham:std_net_impl_debug, r=alexcrichton
70db766 
I'm uncertain whether the 3 implementations in `net2` should unwrap the socket address values. Without unwrapping it looks like this:

```
UdpSocket { addr: Ok(V4(127.0.0.1:34354)), inner: 3 }
TcpListener { addr: Ok(V4(127.0.0.1:9123)), inner: 4 }
TcpStream { addr: Ok(V4(127.0.0.1:9123)), peer: Ok(V4(127.0.0.1:58360)), inner: 5 }
```

One issue is that you can create, e.g. `UdpSocket`s with bad addresses, which means you can't just unwrap in the implementation:

```
#![feature(from_raw_os)]
use std::net::UdpSocket;
use std::os::unix::io::FromRawFd;

let sock: UdpSocket = unsafe { FromRawFd::from_raw_fd(-1) };
println!("{:?}", sock); // prints "UdpSocket { addr: Err(Error { repr: Os(9) }), inner: -1 }"

```

Fixes #23134.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement Debug for std::net::{UdpSocket,TcpStream,TcpListener,Shutdown} nham/rust
5 participants
@carllerche @kmcallister @nham @wesleywiser @sfackler