Implement get_winsize() for unix. #20613
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see CONTRIBUTING.md for more information. |
|
I think it'd be nicer if you had a const TIOCGWINSZ for the magic value. Also, is there a chance the ioctl could fail but write garbage in to the struct? That would currently get past your test but the call to ioctl returns -1 on error which should be a clearer indicator of a problem. |
|
I missed the error return, that would be better. I'm not sure defining the const is necessary for this one use. |
|
Is there any way to write a test for this? |
|
I can't really think of a way to write a test for this since most tests will not take place in a tty. Not to mention if the test did take place in a tty it would have to always be of the same size for the test to work correctly. |
|
@dgriffen Defining the constant gives later readers something to google for. |
| } | ||
|
|
||
| let mut size = winsize { ws_row: 0, ws_col: 0, ws_pixel: 0, ws_ypixel: 0 }; | ||
| if funcs::bsd44::ioctl(0, 0x00005413, &mut size) == -1 { |
There was a problem hiding this comment.
Can you move this literal into a const with a corresponding name that it has in C?
|
Thanks @dgriffen! Just one minor nit (that was also previously mentioned!) and otherwise r=me |
|
Is this supposed to work on OSX? The value of |
| } | ||
|
|
||
| let mut size = winsize { ws_row: 0, ws_col: 0, ws_pixel: 0, ws_ypixel: 0 }; | ||
| if funcs::bsd44::ioctl(0, TIOCGWINSZ, &mut size) == -1 { |
|
ping @dgriffen, any thoughts on @utkarshkukreti's comments? |
|
I don't have a mac to test, so i can't confirm this. But it appears TIOCGWINSZ only has a different value on powerpc macs. I could include the other value with a compiler directive. But i would like someone to confirm the value first. |
|
The value I have locally (OSX 10.10) for TIOCGWINSZ is |
|
Is you mac a powerpc or intel? |
|
I'm using an intel mac |
|
Updated the code with macos value. I cannot confirm that it works on mac, for aforementioned reasons. I have screenshots of a test on arch linux running gnome terminal. |
|
Thanks! Could you squash the commits together as well? |
|
Squashed everything together, should be ready to go. |
| @@ -19,6 +19,12 @@ pub struct TTY { | |||
| pub fd: FileDesc, | |||
| } | |||
|
|
|||
| #[cfg(target_os = "macos")] | |||
| const TIOCGWINSZ: i32 = 0x40087468; | |||
There was a problem hiding this comment.
I think this (and the one below) should be of type c_ulong, not i32. Typo?
|
the ioctl function expects an i32, c_ulong is 64bit on 64bit machines, and has no way of being passes into ioctl. Edit: Did a quick look, it appears that the version of ioctl I was calling expected an i32, found a version at c::iotcl that expects c_ulong, I am compiling and testing now. |
|
Apparently |
|
Also, it looks like openbsd used 0x40087468 as the value for TIOCGWINSZ, so I'll update that as well. |
|
@bors: r+ 063b0f0 |
|
Thanks! |
|
⌛ Testing commit 063b0f0 with merge 7926a5d... |
|
💔 Test failed - auto-linux-64-x-android-t |
|
I'll add android to the list for TIOCGWINSZ. Are there any other targets I need to add? |
|
If I am unable to find a value for a specific os, should i leave code for that os as unimplemented? |
|
I found this locally: Other platforms which are built include iOS and dragonfly. Perhaps this function could only be defined for supported platforms for now? |
|
Made changes to the code that should allow it to compile on all platforms. For now ios and dragonfly remain unimplemented. |
No description provided.