Re: [PATCH v4 1/1] Cygwin: pty: Fix state management for pseudo console support.

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Takashi,

On Sep  3 12:05, Takashi Yano wrote:
> > > -      Sleep (60); /* Wait for pty_master_fwd_thread() */
> > > +      Sleep (20); /* Wait for pty_master_fwd_thread() */
> > 
> > Isn't that a separate issue as well?  A separate patch may be in order
> > here, kind of like "Cygwin: pseudo console: reduce time sleeping ..."
> > with a short description why that makes sense?
> 
> Actually it is not. The wait time became able to be reduced by
> redesigning switching of r/w pipes which managed via variable
> switch_to_pcon. So I think this should be included in this patch. 

Ok.

> > However, I have a few questions in terms of the code in general, namely
> > in terms of
> > 
> >   ALWAYS_USE_PCON
> >   USE_API_HOOK
> >   USE_OWN_NLS_FUNC
> > 
> > Can you describe again why you introduced these macros?
> 
> These are defined for debugging purpose.
> 
> If ALWAYS_USE_PCON is defined to true, pseudo console pipe is used for
> all process including pure cygwin process. Usually, this should be false
> so that the cygwin process use named pipe as previous.
> 
> USE_API_HOOK is for enabling/disabling the API hook to detect direct
> console access in cygwin process. This should be true so that the
> r/w pipe switching is set to pseudo console side for the cygwin
> process which directly access console.
> 
> As for USE_OWN_NLS_FUNC, I have not decided yet which codes should be
> used. If USE_OWN_NLS_FUNC is false, setlocale (LC_CTYPE, "") is
> called therefore it may affect to some programs wihch do not call
> setlocale().
> 
> > In terms of USE_API_HOOK:
> > 
> > - Shouldn't the hook_api function be moved to hookapi.cc?
> 
> I will move it into hookapi.cc, and post it as a separate patch.
> 
> > - Do we really want to hook every invocation of WriteFile/ReadFile?
> >   Doesn't that potentially slow down an exec'ed process a lot?
> >   We're still not using the NT functions throughout outside of the
> >   console/tty code.
> 
> I measured the time for calling WriteFile() 1000000 times writing
> 1 byte to a disk file for each call.

Files are not affected in Cygwin, fortunately.  They use
NtReadFile/NtWriteFile, not ReadFile/WriteFile.  However, other
stuff is still affected...

> Not hooked:
> Total: 4.558321 seconsd
> 
> Hooked:
> Total: 6.522692 seconsd
> 
> Hooking causes slow down indeed. It seems that GetConsoleMode()
> is slow. So I have added the check for GetFileType() before
> GetConsoleMode() and made check in two stages.
> 
> Hooked (new):
> Total: 5.217996 seconsd
> 
> This results in speed up a little. I will post another patch for this.

This is a slowdown of about 15%.  That's quite a lot.  Can't you just
check the incoming handle against the interesting handles somehow?
If there's no other way around it, we should at least make sure (in a
separate patch) that Cygwin calls NtReadFile/NtWriteFile throughout,
except in tty and console code.

> > In terms of USE_OWN_NLS_FUNC:
> > 
> > - Why do we need this function at all?  Can't this be handled by
> >   __loadlocale instead?  If not, what is __loadlocale missing to make
> >   this work without duplicating the function?
> 
> Calling __loadlocale() here causes execution error.
> 
> mintty:
>       0 [main] tcsh 1901 sig_send: error sending signal 6, pid 1901, pipe handle 0x0, nb 0, packsize 164, Win32 error 6
> 
> script:
> Script started, file is typescript
> script: failed to execute /bin/tcsh: Bad address
> Script done, file is typescript
> 
> I could not find out the reason. Some kind of initialization which
> is needed by __loadlocale() may not be done yet. So I copied
> only necessary part from __loadlocale() and nl_langinfo().
> 
> Simply,
>  path_conv a ("/usr/share/locale/locale.alias");
> also causes errors on starting mintty.
> 
> Ideally, the cause of the error should be found out, I suppose.

Indeed.  We can keep the code in for now, but the end result should
call a tweaked version of __loadlocale instead.  As long as the 
tweak only requires a single extra argument, or if the category or
new_locale argument can be used as indicator to trigger the required
special behavour, we should have no problem to get that into newlib.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Attachment: signature.asc
Description: PGP signature