DMARC-Filter: OpenDMARC Filter v1.4.2 delorie.com 5B4B4H9b533231
Date: Thu, 4 Dec 2025 12:01:58 +0100
To: cygwin@cygwin.com
Subject: Re: dynamic conpty loading
Message-ID: <aTFqJo4J6oup4yzH@calimero.vinschen.de>
Mail-Followup-To: cygwin@cygwin.com
References: <30deef29-ba20-49d9-9b38-28020941b0ef@towo.net>
 <707f4d4c-a20d-4f1d-bdbd-65a2392dea15@towo.net>
 <20251121190433.4a37dc959848a0aea89a8922@nifty.ne.jp>
 <62f3c1ae-1fd9-477d-a837-d865094dab06@towo.net>
 <aSCXcoeDbrGOrUTu@calimero.vinschen.de>
 <cc6b613b-1c02-41e3-b045-5b1723b706ab@towo.net>
 <20251126193220.1b890f58a121add2f0d05fa5@nifty.ne.jp>
 <3a6ed5bc-24fd-42b0-9f57-83c8eb678a1b@towo.net>
 <20251127172026.6b6f876fd341f9c0cbd1f876@nifty.ne.jp>
 <20251203175638.0b4f51242c8fdab685ad9b7e@nifty.ne.jp>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <20251203175638.0b4f51242c8fdab685ad9b7e@nifty.ne.jp>
Precedence: list
From: Corinna Vinschen via Cygwin <cygwin@cygwin.com>
Reply-To: cygwin@cygwin.com
Cc: Corinna Vinschen <corinna-cygwin@cygwin.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: cygwin-bounces~archive-cygwin=delorie.com@cygwin.com
Sender: "Cygwin" <cygwin-bounces~archive-cygwin=delorie.com@cygwin.com>

On Dec  3 17:56, Takashi Yano via Cygwin wrote:
> Subject: [PATCH v2] Cygwin: pty: Experimental OpenConsole.exe support

Some commit message here?  Yeah, I know, it's just experimental for now :)

> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>  winsup/cygwin/fhandler/pty.cc | 281 ++++++++++++++++++++++++++++++----
>  1 file changed, 251 insertions(+), 30 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
> index 679068ea2..2c9af1742 100644
> --- a/winsup/cygwin/fhandler/pty.cc
> +++ b/winsup/cygwin/fhandler/pty.cc
> @@ -33,6 +33,136 @@ details. */
>  #define PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE 0x00020016
>  #endif /* PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE */
>  
> +static NTSTATUS
> +create_handle (PHANDLE handle, PCWSTR device_name,
> +	       ACCESS_MASK desired_access, HANDLE parent,
> +	       BOOLEAN inheritable, ULONG open_options)
> +{
> +  ULONG flags = OBJ_CASE_INSENSITIVE;
> +  if (inheritable)
> +    flags |= OBJ_INHERIT;
> +
> +  UNICODE_STRING name;
> +  RtlInitUnicodeString (&name, device_name);
> +
> +  OBJECT_ATTRIBUTES object_attributes;
> +  InitializeObjectAttributes (&object_attributes, &name, flags, parent, NULL);
> +
> +  IO_STATUS_BLOCK io;
> +  return NtOpenFile (handle, desired_access, &object_attributes, &io,
> +		     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> +		     open_options);
> +}
> +
> +extern "C" WINBASEAPI HRESULT WINAPI
> +CreatePseudoConsole_new (COORD size, HANDLE h_input, HANDLE h_output,
> +			 DWORD flags, HPCON *hpcon)
> +{

I'm actually surprised that Cygwin should implement its own
CreatePseudoConsole instead of just trying to call into conpty.dll.
If you think this is the right thing to do, ok with me.

But then, this code is basically by courtesy of the terminal code on
github.  It would be prudent to add a comment preceding the code
explaining where the algorithm is coming from and copying the MIT
license.  Note that attribution is part of the MIT license:
https://opensource.org/license/mit

> +  status = create_handle (&h_con_reference, L"\\Reference",

I would rename the function a bit to make clear this is only for the local
creation of the pseudo console handles.  Sth like create_console_handle or
create_pconsole_hdl, perhaps.

> @@ -2136,6 +2266,8 @@ fhandler_pty_master::close (int flag)
>  ssize_t
>  fhandler_pty_master::write (const void *ptr, size_t len)
>  {

Aren't from here on two patches folded into one?  All but one hunk (the
one calling CreatePseudoConsole_new) seem to be entirely independent of
OpenConsole.

It would be nice to get a commit message explaining the changes and
source comments explaining the new code wouldn't hurt either.

> +  size_t towrite = len;
> +
>    ssize_t ret;
>    char *p = (char *) ptr;
>    termios &ti = tc ()->ti;
> @@ -2152,7 +2284,7 @@ fhandler_pty_master::write (const void *ptr, size_t len)
>  	 If the reply for "CSI6n" is divided into multiple writes,
>  	 pseudo console sometimes does not recognize it.  Therefore,
>  	 put them together into wpbuf and write all at once. */
> -      static const int wpbuf_len = strlen ("\033[32768;32868R");
> +      static const int wpbuf_len = 64; /* for response to CSI6n nad CSIc */

Is there some macro for the count of 64 in this context, by any chance?

> [...]
> if [ $(uname -m) = "x86_64" ]
> then
> 	POSTFIX="x64"
> else
> 	POSTFIX="x86"

Do we really want a 32 bit version?  Isn't there an aarch64 version?


> VERSION=$(cat /etc/libopenconsole/version.txt)
> cd /tmp
> wget -q https://github.com/microsoft/terminal/releases/download/v${VERSION}/Microsoft.WindowsTerminal_${VERSION}_${POSTFIX}.zip -O - > Microsoft.WindowsTerminal_${VERSION}_${POSTFIX}.tmp

For a start, this may be ok, but we should really try to build our own
OpenConsole package build by our own gcc or clang, IMHO.


Corinna

-- 
Problem reports:      https://cygwin.com/problems.html
FAQ:                  https://cygwin.com/faq/
Documentation:        https://cygwin.com/docs.html
Unsubscribe info:     https://cygwin.com/ml/#unsubscribe-simple