unix, windows: add uv_fileno #1435
Conversation
|
This patch is unifinished (Windows part and tests are missing), but I wanted to start a discussion about it. I can't think of a way which would please everyone while keeping our fds hidden, so I think it might not be that bad to expose them for those with legit use cases such as setting some socket options here and there, we can't possibly map all of them. About the Windows side, HANDLE is just a void*, which means we wouldn't be able to cast it to int because of 64bit systems. However, reading around seems like only the lower 32 bits are used because a process can only have 2^24 handles in that handle table. Anyway, I went for the typedef, since this is a platform dependent thing and feels more future-proof. Thoughts? Calling the cavalry! @indutny, @bnoordhuis, @piscisaureus |
|
IMO this is a good idea. Let the user do whatever he wants. |
|
This is such an easy fix, why didn't we have it earlier? |
|
Just like with scotch, some things take time to become better, I guess :-P |
| @@ -615,6 +615,9 @@ UV_EXTERN void uv_close(uv_handle_t* handle, uv_close_cb close_cb); | |||
| */ | |||
| UV_EXTERN int uv_recv_buffer_size(uv_handle_t* handle, int* value); | |||
|
|
|||
| /* TODO */ | |||
| UV_EXTERN uv_os_fd_t uv_fileno(uv_handle_t* handle); | |||
There was a problem hiding this comment.
Maybe UV_EXTERN int uv_file(const uv_handle_t* handle, uv_os_fd_t* fd);? Libuv can't really return a meaningful error now.
There was a problem hiding this comment.
That crossed my mind, but in the end I thought that I'd just return -1 when there is no fd. We could give EINVAL if you pass an unsupported handle type, I guess.
There was a problem hiding this comment.
But how are you going to do that on Windows? HANDLE is a typedef for void*.
There was a problem hiding this comment.
I guess the same way Windows does for INVALID_SOCKET :-P On a more serious note, I do like your proposal, I'll change this.
There was a problem hiding this comment.
FWIW, Windows defines INVALID_SOCKET as SOCKET(~0).
|
Updated the API and Unix implementation as per Ben's suggestions. It returns EINVAL if the handle is not a stream, poll or udp and EBADF if the fd is -1. |
| } | ||
|
|
||
| if (*fd == -1) | ||
| return -EBADF; |
There was a problem hiding this comment.
It's kind of bad style to return an error code but still mutate the out parameter, IMO.
There was a problem hiding this comment.
True that, will fix.
|
Added tests and Windows implementation. The TTY test fails, I need to open $conin it seems, like the TTY test does, will do. |
| ASSERT(r == 0); | ||
| uv_close((uv_handle_t*) &pipe, NULL); | ||
| #ifndef _WIN32 | ||
| /* close is not immediate on Windows */ |
There was a problem hiding this comment.
Can't you filter on UV__HANDLE_CLOSING in uv_fileno()?
There was a problem hiding this comment.
I'm not sure what guarantees we make about it. The handle (libuv wise) is closing, but the actual Windows HANDLE might need to stick around for a bit more.
|
Tests pass on Windows now. |
|
@bnoordhuis thanks, const-ified the handle now. |
|
Any further comments on this? |
|
Why do we set it to invalid? |
Because otherwise you get it with uv_fileno after you have closed it so it might belong to another handle that reused it. This wasn't a problem before, because it was not exposed, but now that people get access to file descriptors, we cannot give them stalled ones. Another possibility might be to return EBADF if uv_is_closing(handle). That might actually simplify things a bit... |
|
I'll give that a shot (I think that's what @bnoordhuis said before, but I didn't quite get it until now). I'd still leave the commit that sets handles to invalid, as a defense mechanism. |
Returns the platform specific file descriptor for handles that are backed by one. The datatype is abstracted as uv_os_fd_t, which maps to int on Unices and HANDLE on Windows. Users can use this function to set specific socket options, for example, in a non portable way. This function is essentially a shotgun, you better be careful with whatever you do with it, don't blame me if you used it to get the fd of a stream, close it yourself and expect things to Just Work.
Set the socket to INVALID_SOCKET foir TCP and UDP handles and the handle to INVALID_HANDLE_VALUE on TTY handles after closing them.
|
Updated with uv_is_closing check, now we no longer need any ifdef-ery on the tests \o/. |
|
LGTM. And yes, that was what I meant. :-) |
|
Thanks folks, landed! Load your shotguns. |
Returns the platform specific file descriptor for handles that are
backed by one. The datatype is abstracted as uv_os_fd_t, which maps to
int on Unices and HANDLE on Windows.
Users can use this function to set specific socket options, for example,
in a non portable way.
This function is essentially a shotgun, you better be careful with
whatever you do with it, don't blame me if you used it to get the fd of
a stream, close it yourself and expect things to Just Work.