Hi Hannes,
On Thu, 22 Dec 2016, Johannes Sixt wrote:
> Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:
> > +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...
>
> > + *
> > + * 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?
You are correct, we must update `console` because `handle` is no longer
the handle we want.
The comment above only meant to reinforce that we have to forget about the
previous handle, too, as we might access something completely different
than a console otherwise.
Would you have a suggestion how to rephrase the comment to make it less
confusing?
Ciao,
Dscho