Re: [PATCH v2 3/3] mingw: replace isatty() hack
flat view
From: Johannes Sixt <hidden>
Date: 2016-12-22 20:26:54
I've only one request for clarification below. Otherwise, the patch looks good. (lines removed by the patch trimmed) Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:
quoted hunk ↗ jump to hunk
+static HANDLE swap_osfhnd(int fd, HANDLE new_handle) +{ + /* + * Create a copy of the original handle associated with fd + * because the original will get closed when we dup2(). + */ + HANDLE handle = (HANDLE)_get_osfhandle(fd); + HANDLE duplicate = duplicate_handle(handle); + /* Create a temp fd associated with the already open "new_handle". */ + int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY); + assert((fd == 1) || (fd == 2)); + /* + * Use stock dup2() to re-bind fd to the new handle. Note that + * this will implicitly close(1) and close both fd=1 and the + * originally associated handle. It will open a new fd=1 and + * call DuplicateHandle() on the handle associated with new_fd. + * It is because of this implicit close() that we created the + * copy of the original. + * + * Note that the OS can recycle HANDLE (numbers) just like it + * recycles fd (numbers), so we must update the cached value + * of "console". You can use GetFileType() to see that + * handle and _get_osfhandle(fd) may have the same number + * value, but they refer to different actual files now.
Certainly, the OS does not recycle handle values that are in use (open). Then I do not quite get the point of this paragraph. See...
quoted hunk ↗ jump to hunk
+ * + * Note that dup2() when given target := {0,1,2} will also + * call SetStdHandle(), so we don't need to worry about that. + */ + dup2(new_fd, fd); + if (console == handle) + console = duplicate;
... This is where "the cached value of console is updated", right? If console == handle, then this is not because a handle value was recycled, but because fd *was* console. Since the old value of console has been closed by the dup2(), we must refer to the back-up value in the future. Am I missing something?
quoted hunk ↗ jump to hunk
+ handle = INVALID_HANDLE_VALUE; + /* Close the temp fd. This explicitly closes "new_handle" + * (because it has been associated with it). + */ + close(new_fd); + fd_is_interactive[fd] |= FD_SWAPPED; + return duplicate; }