Re: [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Dec 1, 2012, 12:43 UTC
Message-ID: <CABPQNSY_vHdvBvU_ezjyOzoZeBJAYTJ2829o6Vxs-nJjQVcvDQ@mail.gmail.com>
In-Reply-To: <alpine.DEB.1.00.1211301858570.31987@s15462909.onlinehome-server.info>

On Fri, Nov 30, 2012 at 6:59 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 25 quoted lines

> Hi,
>
> On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
>
>> By moving the echo-disabling code to a separate function, we can
>> implement OS-specific versions of it for non-POSIX platforms.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>>  compat/terminal.c | 43 +++++++++++++++++++++++++------------------
>>  1 file changed, 25 insertions(+), 18 deletions(-)
>>
>> diff --git a/compat/terminal.c b/compat/terminal.c
>> index bbb038d..3217838 100644
>> --- a/compat/terminal.c
>> +++ b/compat/terminal.c
>> @@ -14,6 +14,7 @@ static void restore_term(void)
>>               return;
>>
>>       tcsetattr(term_fd, TCSAFLUSH, &old_term);
>> +     close(term_fd);
>>       term_fd = -1;
>>  }
>
> That looks like an independent resource leak fix... correct?

It might look like it, but it's not; term_fd used to be returned by "fileno(fh)", and fh did get properly closed.

With my refactoring, disable_echo/restore_term takes opens /dev/tty a second time, like Jeff points out. And that second file descriptor needs to be closed.

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en