win: make uv__get_osfhandle public #1323

jviotti · 2017-04-26T17:46:13Z

The uv__get_osfhandle function is a private functio of the Windows
subsystem, and its used to get a Windows HANDLE out of a file descriptor
number.

The motivation behind making this function public is to allow Node.js
programs to pass file descriptors created using fs.open() to native
Node.js C++ add-ons, and be able to successfully convert them to Windows
HANDLEs.

This commit implements uv_get_osfhandle() in both src/win/handle.c
and src/unix/fs.c, and returns uv_os_fd_t, which is typedef'd to
HANDLE on Windows, and to int otherwise. If called on UNIX based
operating systems, the function will simply return back the file
descriptor number.

The function is then made public on include/uv.h.

This work is based on #1166

Fixes: #1291
See: nodejs/node#6369
Signed-off-by: Juan Cruz Viotti jviotti@openmailbox.org

saghul · 2017-04-27T07:48:48Z

docs/src/fs.rst

+   Note that the return value is still owned by the C runtime,
+   any attempts to close it or to use it after closing the fd may lead to malfunction.
+
+    .. versionadded:: 2.0.0 replace uv_file with uv_os_fd_t


Please adjust the version number. The next release will be 1.12.0

saghul · 2017-04-27T07:49:48Z

Also please double check uv_os_fd_t is what we want here.

@vtjnash can you please review this?

vtjnash · 2017-04-27T14:03:07Z

This is valid, but specifically doesn't fix #1291 (in particular, the comment about 'static inline' at https://github.com/libuv/libuv/pull/1323/files#diff-56343bd7ad8bf449c2693082fb3e4081R447 is the exact opposite of the functionality needed by that issue).

I would recommend dropping most of these changes and adding a simple wrapper function somewhere in the src/win/handle.c file:

uv_os_fd_t uv_get_osfhandle(int fd) {
  return uv__get_osfhandle(fd);
}

and then just add the corresponding prototype to the uv.h header:

UV_EXTERN uv_os_fd_t uv_get_osfhandle(int fd);

jviotti · 2017-04-28T14:54:14Z

@vtjnash Sounds good to me! I'll update the PR in a minute. Just a couple of questions:

Should I still preserve the pre-processor conditional that makes uv_get_osfhandle simply return back fd on unix? Given uv_get_osfhandle lives inside src/win, should I create its unix counterpart instead?
Should we prefix the public function with fs? E.g: uv_fs_get_osfhandle?

jviotti · 2017-04-28T15:01:24Z

Updated!

vtjnash · 2017-04-28T16:37:20Z

This looks correct to me. I can't comment on the naming or formatting of APIs though.

santigimeno

Can we have a test?

mafintosh · 2017-05-01T21:15:15Z

Any chance of getting this in an upcoming release? Using windows specific file apis are tricky without this.

jviotti · 2017-05-02T05:49:19Z

I'll try to add a test for this.

jviotti · 2017-05-02T06:35:41Z

I added a test case that opens a file, writes a test buffer to it, uses uv_get_osfhandle to get a uv_os_fd_t, and then uses the uv_os_fd_t to read back from the file (ensuring that its really valid), and compare the result with the initial buffer. Let me know what you think. I'm sure the test case can be improved!

I wrote the case on macOS, but I'll give it a go on Windows in a minute.

saghul

There are still some required changes here.

saghul · 2017-05-02T07:34:31Z

docs/src/fs.rst

+.. c:function:: uv_os_fd_t uv_get_osfhandle(int fd)
+
+   For a file descriptor in the C runtime, get the OS-dependent handle.
+   On UNIX, returns ``fd``. On Windows, this calls ``_get_osfhandle``.


please mention the fd is returned unchanged on unix.

also add a link to the relevant MSDN page.

saghul · 2017-05-02T07:35:39Z

src/unix/fs.c

@@ -1367,3 +1367,8 @@ void uv_fs_req_cleanup(uv_fs_t* req) {
    uv__free(req->ptr);
  req->ptr = NULL;
 }
+
+
+uv_os_fd_t uv_get_osfhandle(int fd) {


this is not the right place for this function, please move it to core.c

saghul · 2017-05-02T07:37:50Z

test/test-fs.c

+#endif
+
+  iovs[0] = uv_buf_init(buf, sizeof(test_buf));
+  r = uv_fs_read(NULL, &read_req, fd, iovs, 1, 0, NULL);


this won't work on Windows, you must pass the fd, not the handle, use open_req1.result instead.

Hm, yeah, I just realized that, in which case my whole test case doesn't make sense anymore. Can you think of any uv function that interacts with files that could accept a uv_os_fd_t, to ensure its validity? Otherwise, I can reduce the test case to just check that the fd is greater than zero on UNIX, and not an invalid handle on Windows. What do you think?

That should be fine.

I'll update the test name though.

saghul · 2017-05-02T07:38:07Z

test/test-fs.c

+  /* Cleanup. */
+  unlink("test_file");
+
+  return 0;


use MAKE_VALGRIND_HAPPY here.

addaleax · 2017-05-27T11:08:31Z

@jviotti I think you might need to rebase this :)

The `uv__get_osfhandle` function is a private functio of the Windows subsystem, and its used to get a Windows HANDLE out of a file descriptor number. The motivation behind making this function public is to allow Node.js programs to pass file descriptors created using `fs.open()` to native Node.js C++ add-ons, and be able to successfully convert them to Windows HANDLEs. This commit implements `uv_get_osfhandle()` in both `src/win/handle.c` and `src/unix/fs.c`, and returns `uv_os_fd_t`, which is typedef'd to `HANDLE` on Windows, and to `int` otherwise. If called on UNIX based operating systems, the function will simply return back the file descriptor number. The function is then made public on `include/uv.h`. This work is based on #1166 Fixes: #1291 See: nodejs/node#6369 Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>

jviotti · 2017-05-28T03:10:27Z

Thanks for the heads up @addaleax. The conflicts are now resolved.

saghul

LGTM

saghul · 2017-05-29T19:02:30Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/281/

The uv__get_osfhandle() function is a private functio of the Windows subsystem, and its used to get a Windows HANDLE out of a file descriptor number. The motivation behind making this function public is to allow Node.js programs to pass file descriptors created using fs.open() to native Node.js C++ add-ons, and be able to successfully convert them to Windows HANDLEs. Refs: #1166 Refs: nodejs/node#6369 Fixes: #1291 PR-URL: #1323 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

cjihrig · 2017-05-30T15:58:29Z

Landed in e133923. Thanks!

davedoesdev · 2017-07-06T21:14:48Z

Thanks for this fix, works well.

jviotti mentioned this pull request Apr 26, 2017

_get_osfhandle in Addon fails on Windows nodejs/node#6369

Closed

saghul requested changes Apr 27, 2017

View reviewed changes

santigimeno approved these changes Apr 28, 2017

View reviewed changes

saghul requested changes May 2, 2017

View reviewed changes

bzoz approved these changes May 13, 2017

View reviewed changes

baudehlo mentioned this pull request May 26, 2017

npm install fs-ext -g FAILS baudehlo/node-fs-ext#57

Closed

addaleax mentioned this pull request May 27, 2017

win: _get_osfhandle crashes when getting the handle for an fd nodejs/nan#671

Closed

saghul approved these changes May 29, 2017

View reviewed changes

cjihrig closed this May 30, 2017

jviotti deleted the expose-uv-get-osfhandle branch May 30, 2017 16:15

davedoesdev added a commit to davedoesdev/node-windows-fd-test that referenced this pull request Jul 6, 2017

Use uv_get_osfhandle (the fix, libuv/libuv#1323)

f36a8ac

davedoesdev mentioned this pull request Jul 7, 2017

Fixes for Windows baudehlo/node-fs-ext#70

Merged

vtjnash mentioned this pull request Jan 5, 2018

provide uv_umask function #1693

Open

primeare mentioned this pull request Apr 30, 2018

No access to uv_get_osfhandle from N-API nodejs/abi-stable-node#318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

win: make uv__get_osfhandle public #1323

win: make uv__get_osfhandle public #1323

jviotti commented Apr 26, 2017 •

edited

Loading

saghul Apr 27, 2017

saghul commented Apr 27, 2017

vtjnash commented Apr 27, 2017

jviotti commented Apr 28, 2017

jviotti commented Apr 28, 2017

vtjnash commented Apr 28, 2017

santigimeno left a comment

mafintosh commented May 1, 2017

jviotti commented May 2, 2017

jviotti commented May 2, 2017 •

edited

Loading

saghul left a comment

saghul May 2, 2017

saghul May 2, 2017

jviotti May 2, 2017

saghul May 2, 2017

jviotti May 2, 2017

saghul May 2, 2017

jviotti May 2, 2017

saghul May 2, 2017

jviotti May 2, 2017

jviotti May 2, 2017

jviotti May 2, 2017

saghul May 2, 2017

jviotti May 2, 2017

addaleax commented May 27, 2017

jviotti commented May 28, 2017

saghul left a comment

saghul commented May 29, 2017

cjihrig commented May 30, 2017

davedoesdev commented Jul 6, 2017

win: make uv__get_osfhandle public #1323

win: make uv__get_osfhandle public #1323

Conversation

jviotti commented Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

saghul commented Apr 27, 2017

vtjnash commented Apr 27, 2017

jviotti commented Apr 28, 2017

jviotti commented Apr 28, 2017

vtjnash commented Apr 28, 2017

santigimeno left a comment

Choose a reason for hiding this comment

mafintosh commented May 1, 2017

jviotti commented May 2, 2017

jviotti commented May 2, 2017 • edited Loading

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented May 27, 2017

jviotti commented May 28, 2017

saghul left a comment

Choose a reason for hiding this comment

saghul commented May 29, 2017

cjihrig commented May 30, 2017

davedoesdev commented Jul 6, 2017

jviotti commented Apr 26, 2017 •

edited

Loading

jviotti commented May 2, 2017 •

edited

Loading