Re: emacs-27 d66331a: Don't build the Gnulib 'utimens' module on MinGW

Re: emacs-27 d66331a: Don't build the Gnulib 'utimens' module on MinGW

[Robert Pluim]

>>>>> On Mon, 23 Mar 2020 10:38:29 -0400 (EDT), eliz@gnu.org (Eli Zaretskii) said:

    Eli> branch: emacs-27
    Eli> commit d66331aea4d86db17da2a4965e020274e623fda0
    Eli> Author: Eli Zaretskii <eliz@gnu.org>
    Eli> Commit: Eli Zaretskii <eliz@gnu.org>

    Eli>     Don't build the Gnulib 'utimens' module on MinGW
    
    Eli>     * nt/gnulib-cfg.mk (OMIT_GNULIB_MODULE_utimens): Omit the
    Eli>     'utimens' module in the MinGW build: 'utimens' is not used by
    Eli>     Emacs, and 'fdutimens' is implemented in w32.c.

Apropos: we explicitly donʼt use the gnulib 'select' module, even
though it looks like on MSWindows it handles both sockets and
pipes. What functionality is it missing?

Robert

Re: emacs-27 d66331a: Don't build the Gnulib 'utimens' module on MinGW

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 23 Mar 2020 16:26:24 +0100
> 
> Apropos: we explicitly donʼt use the gnulib 'select' module, even
> though it looks like on MSWindows it handles both sockets and
> pipes. What functionality is it missing?

SIGCHLD and C-g interrupts, I guess.  Maybe something else as well, I
don't remember.

Re: emacs-27 d66331a: Don't build the Gnulib 'utimens' module on MinGW

[Robert Pluim]

>>>>> On Mon, 23 Mar 2020 17:52:52 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: Eli Zaretskii <eliz@gnu.org>
    >> Date: Mon, 23 Mar 2020 16:26:24 +0100
    >> 
    >> Apropos: we explicitly donʼt use the gnulib 'select' module, even
    >> though it looks like on MSWindows it handles both sockets and
    >> pipes. What functionality is it missing?

    Eli> SIGCHLD and C-g interrupts, I guess.  Maybe something else as well, I
    Eli> don't remember.

In that case I should go the other direction. How about the following
to remove the 1-char readahead for sockets (which then allows us to
enable datagram sockets on MSWindows).

modified   src/w32.c
@@ -8798,6 +8798,43 @@ _sys_wait_accept (int fd)
   return cp->status;
 }
 
+int
+_sys_wait_readable (int fd)
+{
+  HANDLE hEv;
+  child_process * cp;
+  int rc;
+
+  if (fd < 0 || fd >= MAXDESC)
+    return STATUS_READ_ERROR;
+
+  cp = fd_info[fd].cp;
+
+  if (cp == NULL || cp->fd != fd || cp->status != STATUS_READ_READY)
+    return STATUS_READ_ERROR;
+
+  cp->status = STATUS_READ_FAILED;
+
+  hEv = pfn_WSACreateEvent ();
+  rc = pfn_WSAEventSelect (SOCK_HANDLE (fd), hEv, FD_READ);
+  if (rc != SOCKET_ERROR)
+    {
+      do
+        {
+          rc = WaitForSingleObject (hEv, 500);
+          Sleep (5);
+        } while (rc == WAIT_TIMEOUT
+                 && cp->status != STATUS_READ_ERROR
+                 && cp->char_avail);
+      pfn_WSAEventSelect (SOCK_HANDLE (fd), NULL, 0);
+      if (rc == WAIT_OBJECT_0)
+        cp->status = STATUS_READ_SUCCEEDED;
+    }
+  pfn_WSACloseEvent (hEv);
+
+  return cp->status;
+}
+
 int
 _sys_wait_connect (int fd)
 {
@@ -8923,10 +8960,6 @@ sys_read (int fd, char * buffer, unsigned int count)
 	      return -1;
 
 	    case STATUS_READ_SUCCEEDED:
-	      /* consume read-ahead char */
-	      *buffer++ = cp->chr;
-	      count--;
-	      nchars++;
 	      cp->status = STATUS_READ_ACKNOWLEDGED;
 	      ResetEvent (cp->char_avail);
 
modified   src/w32.h
@@ -175,6 +175,7 @@ #define FILE_SERIAL             0x0800
 
 extern int _sys_read_ahead (int fd);
 extern int _sys_wait_accept (int fd);
+extern int _sys_wait_readable (int fd);
 extern int _sys_wait_connect (int fd);
 
 extern HMODULE w32_delayed_load (Lisp_Object);
modified   src/w32proc.c
@@ -1225,7 +1225,10 @@ reader_thread (void *arg)
       else if (cp->fd >= 0 && (fd_info[cp->fd].flags & FILE_LISTEN) != 0)
 	rc = _sys_wait_accept (cp->fd);
       else
-	rc = _sys_read_ahead (cp->fd);
+        if (fd_info[cp->fd].flags & FILE_SOCKET)
+          rc = _sys_wait_readable (cp->fd);
+        else
+          rc = _sys_read_ahead (cp->fd);
 
       /* Don't bother waiting for the event if we already have been
 	 told to exit by delete_child.  */

Re: emacs-27 d66331a: Don't build the Gnulib 'utimens' module on MinGW

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Tue, 24 Mar 2020 18:07:13 +0100
> 
> In that case I should go the other direction. How about the following
> to remove the 1-char readahead for sockets (which then allows us to
> enable datagram sockets on MSWindows).

I have no idea whether and how the result will differ from what we do
today.  The sheer amount of "remarks" in the MSDN documentation scares
me.  E.g., what happens if the remote end closes the socket,
gracefully or otherwise?

IOW, I'd love someone who _really_ knows about Windows socket
programming to review this and compare with what we do now.  I'm not
that person, sorry.

Re: emacs-27 d66331a: Don't build the Gnulib 'utimens' module on MinGW

[Robert Pluim]

>>>>> On Tue, 24 Mar 2020 20:56:28 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: emacs-devel@gnu.org
    >> Date: Tue, 24 Mar 2020 18:07:13 +0100
    >> 
    >> In that case I should go the other direction. How about the following
    >> to remove the 1-char readahead for sockets (which then allows us to
    >> enable datagram sockets on MSWindows).

    Eli> I have no idea whether and how the result will differ from what we do
    Eli> today.  The sheer amount of "remarks" in the MSDN documentation scares
    Eli> me.  E.g., what happens if the remote end closes the socket,
    Eli> gracefully or otherwise?

We still end up calling read on the socket, just later, so those
error conditions should still be handled. Iʼll note that the gnulib
implementation of select does essentially the same thing (it uses
MsgWaitForMultipleObjects rather than WaitForSingleObject, but I
assume they do much the same thing under the covers).

    Eli> IOW, I'd love someone who _really_ knows about Windows socket
    Eli> programming to review this and compare with what we do now.  I'm not
    Eli> that person, sorry.

Nor am I. Iʼd suggest adding it as a disabled option, but then nobody
would ever use it....

Robert