PATCH: Get rid of superfluous dup() and close() calls.

[Martin Wilck]

patch: dup.diff

Get rid of superfluous dup() and close() calls.

Modified files:
 dlls/kernel/comm.c    |   25 ------------------
 dlls/ntdll/file.c     |    2 -
 dlls/ntdll/virtual.c  |    5 ---
 dlls/winsock/socket.c |   68 +-------------------------------------------------
 files/file.c          |   15 -----------
 files/smb.c           |    1 
 include/async.h       |    1 
 scheduler/client.c    |    1 

Comment:

The dup() in wine_server_handle_to_fd() requires dozens of close()
calls spread over the wine code. I think this is confusing and error-prone
(close() calls are easily forgotten, leading to rapid fd leak - actually, 
when looking through the code, I found a number of functions where I 
had expected a close(), but it was lacking).
Moreover, I can't see why that dup() is necessary (any more). 

If there are still some points in the code where the dup() is really
required, I strongly recommend to insert explicit dup()s in the 
respective functions after calling wine_server_handle_to_fd().

I have carried out a number of IO intensive tests with no problems.
I'd like people to test the COMM port stuff, though, and watch the
number of file descriptors. Other IO- and Network-bound tests would 
be very welcome.

If this is absolutely dumb, please tell me why.

Forwarding to wine-devel, 'cause it's pretty intrusive.

Index: dlls/kernel/comm.c
===================================================================
RCS file: /home/wine/wine/dlls/kernel/comm.c,v
retrieving revision 1.68
diff -u -r1.68 comm.c
--- dlls/kernel/comm.c	25 Oct 2002 21:02:30 -0000	1.68
+++ dlls/kernel/comm.c	24 Jan 2003 14:13:32 -0000
@@ -516,7 +516,6 @@
 		return FALSE;
 	}
 	result = ioctl(fd,TIOCSBRK,0);
-	close(fd);
 	if (result ==-1)
 	  {
 	        TRACE("ioctl failed\n");
@@ -556,7 +555,6 @@
 		return FALSE;
 	}
 	result = ioctl(fd,TIOCCBRK,0);
-	close(fd);
 	if (result ==-1)
 	  {
 	        TRACE("ioctl failed\n");
@@ -598,7 +596,6 @@
 
 	if (tcgetattr(fd,&port) == -1) {
 		COMM_SetCommError(handle,CE_IOE);
-		close(fd);
 		return FALSE;
 	}
 
@@ -670,7 +667,6 @@
 
 	if (!direct)
 	  if (tcsetattr(fd, TCSADRAIN, &port) == -1) {
-		close(fd);
 		COMM_SetCommError(handle,CE_IOE);
 		return FALSE;
 	  } else
@@ -685,7 +681,6 @@
 	    else
 	      result = TRUE;
 	  }
-	close(fd);
 	return result;
 }
 
@@ -726,7 +721,6 @@
          tcflush(fd,TCOFLUSH);
      if(flags&PURGE_RXCLEAR)
          tcflush(fd,TCIFLUSH);
-     close(fd);
 
      return 1;
 }
@@ -782,8 +776,6 @@
 	      handle, lpStat->cbInQue, lpStat->cbOutQue);
     }
 
-    close(fd);
-
     COMM_GetCommError(handle, errors);
     COMM_SetCommError(handle, 0);
 
@@ -817,7 +809,6 @@
 	FIXME("handle %p not found?\n",handle);
         return FALSE;
     }
-    close(fd);
     return TRUE;
 }
 
@@ -916,7 +907,6 @@
      if ((tcgetattr(fd,&port)) == -1) {
          int save_error = errno;
          COMM_SetCommError(handle,CE_IOE);
-         close( fd );
          ERR("tcgetattr error '%s'\n", strerror(save_error));
          return FALSE;
      }
@@ -1023,7 +1013,6 @@
 

                         COMM_SetCommError(handle,IE_BAUDRATE);
-			close( fd );
 			ERR("baudrate %ld\n",lpdcb->BaudRate);
 			return FALSE;
 	}
@@ -1089,7 +1078,6 @@
 #endif
                 default:
                         COMM_SetCommError(handle,IE_BAUDRATE);
-                        close( fd );
 			ERR("baudrate %ld\n",lpdcb->BaudRate);
                         return FALSE;
         }
@@ -1132,7 +1120,6 @@
                             port.c_iflag &= ~INPCK;
                         } else {
                             COMM_SetCommError(handle,IE_BYTESIZE);
-                            close( fd );
                             ERR("Cannot set MARK Parity\n");
                             return FALSE;
                         }
@@ -1143,7 +1130,6 @@
                             port.c_iflag &= ~INPCK;
                         } else {
                             COMM_SetCommError(handle,IE_BYTESIZE);
-                            close( fd );
                             ERR("Cannot set SPACE Parity\n");
                             return FALSE;
                         }
@@ -1151,7 +1137,6 @@
 #endif
                default:
                         COMM_SetCommError(handle,IE_BYTESIZE);
-                        close( fd );
 			ERR("Parity\n");
                         return FALSE;
         }
@@ -1173,7 +1158,6 @@
 			break;
 		default:
                         COMM_SetCommError(handle,IE_BYTESIZE);
-                        close( fd );
 			ERR("ByteSize\n");
 			return FALSE;
 	}
@@ -1188,7 +1172,6 @@
 				break;
 		default:
                         COMM_SetCommError(handle,IE_BYTESIZE);
-                        close( fd );
 			ERR("StopBits\n");
 			return FALSE;
 	}
@@ -1219,12 +1202,10 @@
 	if (tcsetattr(fd,TCSANOW,&port)==-1) { /* otherwise it hangs with pending input*/
 	        int save_error=errno;
                 COMM_SetCommError(handle,CE_IOE);
-                close( fd );
                 ERR("tcsetattr error '%s'\n", strerror(save_error));
 		return FALSE;
 	} else {
                 COMM_SetCommError(handle,0);
-                close( fd );
 		return TRUE;
 	}
 }
@@ -1262,10 +1243,8 @@
                 int save_error=errno;
                 ERR("tcgetattr error '%s'\n", strerror(save_error));
                 COMM_SetCommError(handle,CE_IOE);
-                close( fd );
 		return FALSE;
 	}
-     close( fd );
 #ifndef __EMX__
 #ifdef CBAUD
      speed= (port.c_cflag & CBAUD);
@@ -1465,7 +1444,6 @@
     else
     {
         r = (1 == write(fd, &chTransmit, 1));
-        close(fd);
     }
 
     return r;
@@ -1593,7 +1571,6 @@
         FIXME("tcsetattr on fd %d failed!\n",fd);
         return FALSE;
     }
-    close(fd);
     return TRUE;
 }
 
@@ -1619,7 +1596,6 @@
 	if(fd<0)
 		return FALSE;
 	result = ioctl(fd, TIOCMGET, &mstat);
-	close(fd);
 	if (result == -1)
 	  {
 	    WARN("ioctl failed\n");
@@ -1702,7 +1678,6 @@
     ovp = (async_commio*) HeapAlloc(GetProcessHeap(), 0, sizeof (async_commio));
     if(!ovp)
     {
-        close(fd);
         return FALSE;
     }
 
Index: dlls/ntdll/file.c
===================================================================
RCS file: /home/wine/wine/dlls/ntdll/file.c,v
retrieving revision 1.20
diff -u -r1.20 file.c
--- dlls/ntdll/file.c	7 Jan 2003 20:36:28 -0000	1.20
+++ dlls/ntdll/file.c	24 Jan 2003 14:13:35 -0000
@@ -237,8 +237,6 @@
 	}
 	while ( (result == -1) && ((errno == EAGAIN) || (errno == EINTR)) );
 
-	close( fd );
-
 	if (result == -1)
 	{
 		return IoStatusBlock->u.Status = NTFILE_errno_to_status(errno);
Index: dlls/ntdll/virtual.c
===================================================================
RCS file: /home/wine/wine/dlls/ntdll/virtual.c,v
retrieving revision 1.4
diff -u -r1.4 virtual.c
--- dlls/ntdll/virtual.c	7 Jan 2003 20:36:28 -0000	1.4
+++ dlls/ntdll/virtual.c	24 Jan 2003 14:13:39 -0000
@@ -736,13 +736,11 @@
         VIRTUAL_SetProt( view, ptr + sec->VirtualAddress, size, vprot );
     }
 
-    close( fd );
     *addr_ptr = ptr;
     return STATUS_SUCCESS;
 
  error:
     if (ptr != (char *)-1) munmap( ptr, total_size );
-    close( fd );
     return status;
 }
 
@@ -1408,7 +1406,6 @@
         }
         res = map_image( handle, unix_handle, base, size_low, header_size,
                          shared_fd, shared_size, removable, addr_ptr );
-        if (shared_fd != -1) close( shared_fd );
         if (!res) *size_ptr = size_low;
         return res;
     }
@@ -1484,12 +1481,10 @@
         res = STATUS_NO_MEMORY;
         goto error;
     }
-    if (unix_handle != -1) close( unix_handle );
     *size_ptr = size;
     return STATUS_SUCCESS;
 
 error:
-    if (unix_handle != -1) close( unix_handle );
     if (ptr != (void *)-1) munmap( ptr, size );
     return res;
 }
Index: dlls/winsock/socket.c
===================================================================
RCS file: /home/wine/wine/dlls/winsock/socket.c,v
retrieving revision 1.117
diff -u -r1.117 socket.c
--- dlls/winsock/socket.c	23 Jan 2003 21:20:36 -0000	1.117
+++ dlls/winsock/socket.c	24 Jan 2003 14:13:51 -0000
@@ -336,7 +336,6 @@
     if ( ( (access & GENERIC_READ)  && (*flags & FD_FLAG_RECV_SHUTDOWN ) ) ||
          ( (access & GENERIC_WRITE) && (*flags & FD_FLAG_SEND_SHUTDOWN ) ) )
     {
-        close (fd);
         WSASetLastError ( WSAESHUTDOWN );
         return -1;
     }
@@ -578,7 +577,6 @@
 			 else
 			     wsfds16->fd_array[j++] = wsfds16->fd_array[i];
 		}
-		close(fd);
 		lfd[i] = -1;
 	    }
 	}
@@ -593,24 +591,6 @@
     return num_err;
 }
 
-static void fd_set_unimport( void* wsfds, int lfd[], BOOL b32 )
-{
-    if ( wsfds )
-    {
-#define wsfds16 ((ws_fd_set16*)wsfds)
-#define wsfds32 ((WS_fd_set*)wsfds)
-	int i, count = (b32) ? wsfds32->fd_count : wsfds16->fd_count;
-
-	for( i = 0; i < count; i++ )
-	    if ( lfd[i] >= 0 )
-		close(lfd[i]);
-
-	TRACE("\n");
-#undef wsfds32
-#undef wsfds16
-    }
-}
-
 static int do_block( int fd, int mask )
 {
     fd_set fds[3];
@@ -1417,7 +1397,6 @@
 	    SetLastError(_get_sock_error(s, FD_ACCEPT_BIT));
 	    /* FIXME: care about the error? */
 	}
-        close(fd);
         SERVER_START_REQ( accept_socket )
         {
             req->lhandle = SOCKET2HANDLE(s);
@@ -1516,7 +1495,6 @@
                 ws_sockaddr_free(uaddr,name);
             }
         }
-        close(fd);
     }
     else
     {
@@ -1613,7 +1591,6 @@
         {
             SetLastError(wsaErrno());
         }
-        close(fd);
     }
     else
     {
@@ -1622,7 +1599,6 @@
     return SOCKET_ERROR;
 
 connect_success:
-    close(fd);
     _enable_event(SOCKET2HANDLE(s), FD_CONNECT|FD_READ|FD_WRITE,
                   FD_WINE_CONNECTED|FD_READ|FD_WRITE,
                   FD_CONNECT|FD_WINE_LISTENING);
@@ -1690,7 +1666,6 @@
             res=0;
         }
         ws_sockaddr_free(uaddr,name);
-        close(fd);
     }
     else
     {
@@ -1755,7 +1730,6 @@
         {
             res=0;
         }
-        close(fd);
     }
     else
     {
@@ -1820,12 +1794,10 @@
         } else {
 	    if (getsockopt(fd, (int) level, optname, optval, optlen) == 0 )
 	    {
-		close(fd);
 		return 0;
 	    }
 	    SetLastError((errno == EBADF) ? WSAENOTSOCK : wsaErrno());
 	}
-	close(fd);
     }
     return SOCKET_ERROR;
 }
@@ -1966,7 +1938,6 @@
             if (numInt < 0)
             {
                ERR ("Unable to open /proc filesystem to determine number of network interfaces!\n");
-               close(fd);
                WSASetLastError(WSAEINVAL);
                return (SOCKET_ERROR);
             }
@@ -1976,7 +1947,6 @@
                if (!WSAIOCTL_GetInterfaceName(i, ifName))
                {
                   ERR ("Error parsing /proc filesystem!\n");
-                  close(fd);
                   WSASetLastError(WSAEINVAL);
                   return (SOCKET_ERROR);
                }
@@ -1988,7 +1958,6 @@
                if (ioctl(fd, SIOCGIFADDR, &ifInfo) < 0)
                {
                   ERR ("Error obtaining IP address\n");
-                  close(fd);
                   WSASetLastError(WSAEINVAL);
                   return (SOCKET_ERROR);
                }
@@ -2006,7 +1975,6 @@
                if (ioctl(fd, SIOCGIFBRDADDR, &ifInfo) < 0)
                {
                   ERR ("Error obtaining Broadcast IP address\n");
-                  close(fd);
                   WSASetLastError(WSAEINVAL);
                   return (SOCKET_ERROR);
                }
@@ -2024,7 +1992,6 @@
                if (ioctl(fd, SIOCGIFNETMASK, &ifInfo) < 0)
                {
                   ERR ("Error obtaining Subnet IP address\n");
-                  close(fd);
                   WSASetLastError(WSAEINVAL);
                   return (SOCKET_ERROR);
                }
@@ -2059,7 +2026,6 @@
                if (ioctl(fd, SIOCGIFFLAGS, &ifInfo) < 0)
                {
                   ERR ("Error obtaining status flags for socket!\n");
-                  close(fd);
                   WSASetLastError(WSAEINVAL);
                   return (SOCKET_ERROR);
                }
@@ -2079,14 +2045,12 @@
          default:
          {
             WARN("\tunsupported WS_IOCTL cmd (%08lx)\n", dwIoControlCode);
-            close(fd);
             WSASetLastError(WSAEOPNOTSUPP);
             return (SOCKET_ERROR);
          }
       }
 
       /* Function executed with no errors */
-      close(fd);
       return (0);
    }
    else
@@ -2242,14 +2206,11 @@
 		{
 		    /* AsyncSelect()'ed sockets are always nonblocking */
 		    if (*argp) {
-			close(fd);
 			return 0;
 		    }
 		    SetLastError(WSAEINVAL);
-		    close(fd);
 		    return SOCKET_ERROR;
 		}
-		close(fd);
 		if (*argp)
 		    _enable_event(SOCKET2HANDLE(s), 0, FD_WINE_NONBLOCKING, 0);
 		else
@@ -2279,11 +2240,9 @@
     }
     if( ioctl(fd, newcmd, (char*)argp ) == 0 )
     {
-	close(fd);
 	return 0;
     }
     SetLastError((errno == EBADF) ? WSAENOTSOCK : wsaErrno());
-    close(fd);
   }
   return SOCKET_ERROR;
 }
@@ -2309,7 +2268,6 @@
     {
 	if (listen(fd, backlog) == 0)
 	{
-	    close(fd);
 	    _enable_event(SOCKET2HANDLE(s), FD_ACCEPT,
 			  FD_WINE_LISTENING,
 			  FD_CONNECT|FD_WINE_CONNECTED);
@@ -2437,7 +2395,6 @@
                     else
                         wsfds16->fd_array[j++] = wsfds16->fd_array[i];
                 }
-                if( fd >= 0 ) close(fd);
                 exceptfd[i] = -1;
             }
             if( b32 )
@@ -2449,9 +2406,6 @@
         }
         return highfd;
     }
-    fd_set_unimport(ws_readfds, readfd, b32);
-    fd_set_unimport(ws_writefds, writefd, b32);
-    fd_set_unimport(ws_exceptfds, exceptfd, b32);
     if( ws_readfds ) ((WS_fd_set*)ws_readfds)->fd_count = 0;
     if( ws_writefds ) ((WS_fd_set*)ws_writefds)->fd_count = 0;
     if( ws_exceptfds ) ((WS_fd_set*)ws_exceptfds)->fd_count = 0;
@@ -2553,7 +2507,7 @@
     if ( !iovec )
     {
         err = WSAEFAULT;
-        goto err_close;
+        goto error;
     }
 
     for ( i = 0; i < dwBufferCount; i++ )
@@ -2617,15 +2571,11 @@
     *lpNumberOfBytesSent = n;
 
     HeapFree ( GetProcessHeap(), 0, iovec );
-    close ( fd );
     return 0;
 
 err_free:
     HeapFree ( GetProcessHeap(), 0, iovec );
 
-err_close:
-    close ( fd );
-
 error:
     WARN (" -> ERROR %d\n", err);
     WSASetLastError (err);
@@ -2725,7 +2675,6 @@
         }else{
             if (!convert_sockopt(&level, &optname)) {
 		SetLastError(WSAENOPROTOOPT);
-		close(fd);
 		return SOCKET_ERROR;
 	    }
             if (optname == SO_LINGER && optval) {
@@ -2755,7 +2704,6 @@
                    WARN("SO_RCVTIMEO for %d bytes: assuming unixism\n", optlen);
 		} else {
                    WARN("SO_RCVTIMEO for %d bytes is weird: ignored\n", optlen);
-		    close(fd);
 		    return 0;
 		}
 	    }
@@ -2774,7 +2722,6 @@
                    WARN("SO_SNDTIMEO for %d bytes: assuming unixism\n", optlen);
                } else {
                    WARN("SO_SNDTIMEO for %d bytes is weird: ignored\n", optlen);
-                   close(fd);
                    return 0;
                }
            }
@@ -2782,17 +2729,14 @@
 	}
         if(optname == SO_RCVBUF && *(int*)optval < 2048) {
             WARN("SO_RCVBF for %d bytes is too small: ignored\n", *(int*)optval );
-            close( fd);
             return 0;
         }
 
 	if (setsockopt(fd, level, optname, optval, optlen) == 0)
 	{
-	    close(fd);
 	    return 0;
 	}
 	SetLastError(wsaErrno());
-	close(fd);
     }
     else SetLastError(WSAENOTSOCK);
     return SOCKET_ERROR;
@@ -2859,7 +2803,6 @@
             err = WS2_register_async_shutdown ( s, fd0, ASYNC_TYPE_READ );
             if ( err )
             {
-                close ( fd0 );
                 goto error;
             }
         }
@@ -2868,7 +2811,6 @@
             err = WS2_register_async_shutdown ( s, fd1, ASYNC_TYPE_WRITE );
             if ( err )
             {
-                close ( fd1 );
                 goto error;
             }
         }
@@ -2878,10 +2820,8 @@
         if ( shutdown( fd, how ) )
         {
             err = wsaErrno ();
-            close ( fd );
             goto error;
         }
-        close(fd);
     }
 
     _enable_event( SOCKET2HANDLE(s), 0, 0, clear_flags );
@@ -4006,7 +3946,7 @@
     if ( !iovec )
     {
         err = WSAEFAULT;
-        goto err_close;
+        goto error;
     }
 
     for (i = 0; i < dwBufferCount; i++)
@@ -4070,16 +4010,12 @@
     *lpNumberOfBytesRecvd = n;
 
     HeapFree (GetProcessHeap(), 0, iovec);
-    close(fd);
     _enable_event(SOCKET2HANDLE(s), FD_READ, 0, 0);
 
     return 0;
 
 err_free:
     HeapFree (GetProcessHeap(), 0, iovec);
-
-err_close:
-    close (fd);
 
 error:
     WARN(" -> ERROR %d\n", err);
Index: files/file.c
===================================================================
RCS file: /home/wine/wine/files/file.c,v
retrieving revision 1.177
diff -u -r1.177 file.c
--- files/file.c	11 Jan 2003 21:03:18 -0000	1.177
+++ files/file.c	24 Jan 2003 14:13:59 -0000
@@ -333,7 +333,6 @@
     else if (((access & GENERIC_READ)  && (flags & FD_FLAG_RECV_SHUTDOWN)) ||
              ((access & GENERIC_WRITE) && (flags & FD_FLAG_SEND_SHUTDOWN)))
     {
-        close (fd);
         SetLastError ( ERROR_PIPE_NOT_CONNECTED );
         return -1;
     }
@@ -1755,7 +1754,6 @@
     return !register_new_async (&ovp->async);
 
 error:
-    close (fd);
     return FALSE;
 
 }
@@ -1813,12 +1811,10 @@
         if ( (overlapped==NULL) || NtResetEvent( overlapped->hEvent, NULL ) )
         {
             TRACE("Overlapped not specified or invalid event flag\n");
-	    close(unix_handle);
             SetLastError(ERROR_INVALID_PARAMETER);
             return FALSE;
         }
 
-        close(unix_handle);
         overlapped->InternalHigh = 0;
 
         if(!FILE_ReadFileEx(hFile, buffer, bytesToRead, overlapped, NULL, overlapped->hEvent))
@@ -1835,7 +1831,6 @@
     }
     if (flags & FD_FLAG_TIMEOUT)
     {
-        close(unix_handle);
         return FILE_TimeoutRead(hFile, buffer, bytesToRead, bytesRead);
     }
     switch(type)
@@ -1856,7 +1851,6 @@
                                                              &highOffset, FILE_BEGIN)) &&
                  (GetLastError() != NO_ERROR) )
             {
-              close(unix_handle);
               return FALSE;
             }
         }
@@ -1872,7 +1866,6 @@
 	off_t offset = OVERLAPPED_OFFSET(overlapped);
 	if(lseek(unix_handle, offset, SEEK_SET) == -1)
 	{
-	    close(unix_handle);
 	    SetLastError(ERROR_INVALID_PARAMETER);
 	    return FALSE;
 	}
@@ -1886,7 +1879,6 @@
 	FILE_SetDosError();
 	break;
     }
-    close( unix_handle );
     if (result == -1) return FALSE;
     if (bytesRead) *bytesRead = result;
     return TRUE;
@@ -1998,7 +1990,6 @@
     return !register_new_async (&ovp->async);
 
 error:
-    close (fd);
     return FALSE;
 }
 
@@ -2037,12 +2028,10 @@
         if ( (overlapped==NULL) || NtResetEvent( overlapped->hEvent, NULL ) )
         {
             TRACE("Overlapped not specified or invalid event flag\n");
-	    close(unix_handle);
             SetLastError(ERROR_INVALID_PARAMETER);
             return FALSE;
         }
 
-        close(unix_handle);
         overlapped->InternalHigh = 0;
 
         if(!FILE_WriteFileEx(hFile, buffer, bytesToWrite, overlapped, NULL, overlapped->hEvent))
@@ -2075,7 +2064,6 @@
                                                              &highOffset, FILE_BEGIN)) &&
                  (GetLastError() != NO_ERROR) )
             {
-              close(unix_handle);
               return FALSE;
             }
         }
@@ -2086,7 +2074,6 @@
             return FALSE;
         if (overlapped)
         {
-            close(unix_handle);
             SetLastError(ERROR_INVALID_PARAMETER);
             return FALSE;
         }
@@ -2098,7 +2085,6 @@
 	off_t offset = OVERLAPPED_OFFSET(overlapped);
 	if(lseek(unix_handle, offset, SEEK_SET) == -1)
 	{
-	    close(unix_handle);
 	    SetLastError(ERROR_INVALID_PARAMETER);
 	    return FALSE;
 	}
@@ -2115,7 +2101,6 @@
         FILE_SetDosError();
         break;
     }
-    close( unix_handle );
     if (result == -1) return FALSE;
     if (bytesWritten) *bytesWritten = result;
     return TRUE;
Index: files/smb.c
===================================================================
RCS file: /home/wine/wine/files/smb.c,v
retrieving revision 1.16
diff -u -r1.16 smb.c
--- files/smb.c	24 Jan 2003 00:54:58 -0000	1.16
+++ files/smb.c	24 Jan 2003 14:14:02 -0000
@@ -1532,7 +1532,6 @@
         if(total>=bytesToRead)
             break;
     }
-    close(fd);
 
     if(bytesRead)
         *bytesRead = total;
Index: include/async.h
===================================================================
RCS file: /home/wine/wine/include/async.h,v
retrieving revision 1.5
diff -u -r1.5 async.h
--- include/async.h	4 May 2002 18:37:08 -0000	1.5
+++ include/async.h	24 Jan 2003 14:14:02 -0000
@@ -72,7 +72,6 @@
 
     ovp->next = ovp->prev = NULL;
 
-    close( ovp->fd );
     if( ovp->event != INVALID_HANDLE_VALUE )
         NtSetEvent( ovp->event, NULL );
 
Index: scheduler/client.c
===================================================================
RCS file: /home/wine/wine/scheduler/client.c,v
retrieving revision 1.92
diff -u -r1.92 client.c
--- scheduler/client.c	31 Oct 2002 03:41:56 -0000	1.92
+++ scheduler/client.c	24 Jan 2003 14:14:05 -0000
@@ -436,7 +436,6 @@
          */
     }
 
-    if ((fd != -1) && ((fd = dup(fd)) == -1)) return STATUS_TOO_MANY_OPENED_FILES;
     *unix_fd = fd;
     return STATUS_SUCCESS;
 }
-- 
Martin Wilck                Phone: [phone]
Fujitsu Siemens Computers   Fax:   [phone]
Heinz-Nixdorf-Ring 1	    mailto:Martin.Wilck(a)Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy

[Alexandre Julliard]

Martin Wilck <Martin.Wilck(a)fujitsu-siemens.com> writes:
> The dup() in wine_server_handle_to_fd() requires dozens of close()
> calls spread over the wine code. I think this is confusing and error-prone
> (close() calls are easily forgotten, leading to rapid fd leak - actually, 
> when looking through the code, I found a number of functions where I 
> had expected a close(), but it was lacking).
> Moreover, I can't see why that dup() is necessary (any more).

I think it's cleaner to return a dup of the fd instead of relying on
the fact that the fd is cached internally. This may not always be the
case, and it's better to risk an fd leak than to risk invalidating the
cached value in case someone closes the fd when they shouldn't.

-- 
Alexandre Julliard
julliard(a)winehq.com

[Shachar Shemesh]

Alexandre Julliard wrote:
> ... and it's better to risk an fd leak than to risk invalidating the
> cached value in case someone closes the fd when they shouldn't.

I'm not sure I agree. When someone closes a fd they shouldn't, that 
would lead to program crashing, and attention being brought to the 
problem. When someone leaks a fd, noone will notice.

            Shachar

[David Laight]

> I'm not sure I agree. When someone closes a fd they shouldn't, that 
> would lead to program crashing, and attention being brought to the 
> problem. When someone leaks a fd, noone will notice.

No, it causes horrid corruptions that are particularly difficult
to find.

What happens is that someone else does an open() and is given
the number of the (incorrectly) closed fd.  The owener of the fd
will then write into the newly opened file.

This can happen if a 'normal' program does close(0), close(1),
close(2) then much later accidentally calls printf() instead
of sprintf().  When the stdio buffer is eventually flushed data
is written to what has been reopened as fd0.
(This was a real bug in software that got released...)

	David

-- 
David Laight: david(a)l8s.co.uk

[Shachar Shemesh]

David Laight wrote:
> > I'm not sure I agree. When someone closes a fd they shouldn't, that 
> > would lead to program crashing, and attention being brought to the 
> > problem. When someone leaks a fd, noone will notice.
>
> No, it causes horrid corruptions that are particularly difficult
> to find.
>
> What happens is that someone else does an open() and is given
> the number of the (incorrectly) closed fd.  The owener of the fd
> will then write into the newly opened file.
>
> This can happen if a 'normal' program does close(0), close(1),
> close(2) then much later accidentally calls printf() instead
> of sprintf().  When the stdio buffer is eventually flushed data
> is written to what has been reopened as fd0.
> (This was a real bug in software that got released...)
>
> David

What happens under Windows with such circumstances?

            Sh.

[David Laight]

> What happens under Windows with such circumstances?

Who knows, I've never used windows (well almost never),
people don't seem to mind windows locking solid :-)

	David

-- 
David Laight: david(a)l8s.co.uk

[Martin Wilck]

Am Sam, 2003-02-01 um 10.53 schrieb David Laight:
> No, it causes horrid corruptions that are particularly difficult
> to find.
>
> What happens is that someone else does an open() and is given
> the number of the (incorrectly) closed fd.  The owener of the fd
> will then write into the newly opened file.

Why is that relevant to Wine? 99% of the Wine code uses DOS/Windows
functions like WriteFile() anyway. All we need to do is make sure that
these functions handle the Unix fd's properly (and they will if they
don't call close()). No user code can ever access the Unix fd's.

I admit my patch isn't small but it also shows that the close() calls
are only in half a dozen source files. In the code that is using the
Unix fd's, however, the close() calls are confusing, ugly, and easy to
forget. 

Martin

-- 
Martin Wilck                Phone: [phone]
Fujitsu Siemens Computers   Fax:   [phone]
Heinz-Nixdorf-Ring 1	    mailto:Martin.Wilck(a)Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy

[Martin Wilck]

On Mon, 2003-02-03, I wrote:
> Why is that relevant to Wine? 99% of the Wine code uses DOS/Windows
> functions like WriteFile() anyway. All we need to do is make sure that
> these functions handle the Unix fd's properly (and they will if they
> don't call close()).

What's up, people? I think at least I deserve an answer to this
argument. Or is everybody just too busy?

I am not going to give up easily on this one, because I still think it's
the right thing to do. If anybody is worried about regressions or the
risk of future close() calls, I am willing to care for that.

As for the risk of someone inadvertently close()ing an fd and someone
else writing to it, how about something like this:

static inline int wine_close_unix_fd (int fd) { return close (fd); }
#define close(fd) ERROR --- please dont call close() in Wine !!

in the Wine headers, and changing close() to wine_close_unix_fd()
wherever the close() is found to be legitimate?

Martin

-- 
Martin Wilck                Phone: [phone]
Fujitsu Siemens Computers   Fax:   [phone]
Heinz-Nixdorf-Ring 1	    mailto:Martin.Wilck(a)Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy

[Michael Stefaniuc]

On Thu, Feb 06, 2003 at 10:49:17AM +0100, Martin Wilck wrote:
> On Mon, 2003-02-03, I wrote:
> > Why is that relevant to Wine? 99% of the Wine code uses DOS/Windows
> > functions like WriteFile() anyway. All we need to do is make sure that
> > these functions handle the Unix fd's properly (and they will if they
> > don't call close()).
>
> What's up, people? I think at least I deserve an answer to this
> argument. Or is everybody just too busy?
> Well, i agree with you that the close call's shouldn't be needed but my
> voice dosn't count.

bye
	michael
> I am not going to give up easily on this one, because I still think it's
> the right thing to do. If anybody is worried about regressions or the
> risk of future close() calls, I am willing to care for that.
>
> As for the risk of someone inadvertently close()ing an fd and someone
> else writing to it, how about something like this:
>
> static inline int wine_close_unix_fd (int fd) { return close (fd); }
> #define close(fd) ERROR --- please dont call close() in Wine !!
>
> in the Wine headers, and changing close() to wine_close_unix_fd()
> wherever the close() is found to be legitimate?

-- 
Michael Stefaniuc               Tel.: [phone]
System Administration           Fax.: [phone]
Red Hat GmbH                    Email: mstefani(a)redhat.com
Hauptstaetterstr. 58            http://www.redhat.de/
D-70178 Stuttgart

[Martin Wilck]

Just a side issue: On my system (PIII 1GHz), one pair of dup()/close()
calls takes about 1us. Not all too much, but it may matter in
IO-intensive applications.

Of course I am not arguing in favor of sacrificing stability for a tiny
bit of performance. Just another small pro for my patch :-)

Martin

-- 
Martin Wilck                Phone: [phone]
Fujitsu Siemens Computers   Fax:   [phone]
Heinz-Nixdorf-Ring 1	    mailto:Martin.Wilck(a)Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy

[Dmitry Timoshkov]

"Martin Wilck" <Martin.Wilck(a)Fujitsu-Siemens.com> wrote:
> > Why is that relevant to Wine? 99% of the Wine code uses DOS/Windows
> > functions like WriteFile() anyway. All we need to do is make sure that
> > these functions handle the Unix fd's properly (and they will if they
> > don't call close()).
>
> What's up, people? I think at least I deserve an answer to this
> argument. Or is everybody just too busy?

Martin, you're of course deserving not only an answer but also a lot
of gratitude for your continues work on Wine. But unfortunately AFAIK
Alexandre, who can make a final decision on this, currently has no
reliable internet connection (for already about a week). Please wait
a bit more. Thanks.

-- 
Dmitry.

[Mike McCormack]

Hi Martin,

OK, what happens if one thread closes an FD in the middle of another 
thread's read operation (that might take multiple accesses to an FD)?  I 
think that's unlikely, but that seems to me to be what the dup/close 
business is all about.

It might be that the previous dup/close code had some similar bugs in 
it, but it handles the above problem in a well known way.

Another way to solve the problem would be to enter a critical section 
instead of dup'ing a handle, but that also has a cost, and somebody will 
forget to leave the critical section somewhere in the code...

Mike


Martin Wilck wrote:
> On Mon, 2003-02-03, I wrote:
> > Why is that relevant to Wine? 99% of the Wine code uses DOS/Windows
> > functions like WriteFile() anyway. All we need to do is make sure that
> > these functions handle the Unix fd's properly (and they will if they
> > don't call close()).
>
> What's up, people? I think at least I deserve an answer to this
> argument. Or is everybody just too busy?

[Martin Wilck]

Am Don, 2003-02-06 um 23.50 schrieb Mike McCormack:
> OK, what happens if one thread closes an FD in the middle of another 
> thread's read operation (that might take multiple accesses to an FD)?  I 
> think that's unlikely, but that seems to me to be what the dup/close 
> business is all about.

That'd be impossible if close() calls were prohibited. I entirely agree
that the handling of Unix fd's must be up to the Wine core. But *because
of that* I despise the close() calls spread all about our code.
However, see my answer to Alexandre's mail for a constructive proposal.
> Another way to solve the problem would be to enter a critical section 
> instead of dup'ing a handle, but that also has a cost, and somebody will 
> forget to leave the critical section somewhere in the code...

I don't think that is an option. That would mean no two threads could
simultaneously read and write. A usage count on an fd would be possible.

Martin

-- 
Martin Wilck                Phone: [phone]
Fujitsu Siemens Computers   Fax:   [phone]
Heinz-Nixdorf-Ring 1	    mailto:Martin.Wilck(a)Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy

[Martin Wilck]

Am Fre, 2003-01-31 um 22.37 schrieb Alexandre Julliard:
> I think it's cleaner to return a dup of the fd instead of relying on
> the fact that the fd is cached internally. This may not always be the
> case, and it's better to risk an fd leak than to risk invalidating the
> cached value in case someone closes the fd when they shouldn't.

I see your point, but I don't see the risk you're talking about.

The new coding rule (after removing the dup) is very simple: 

   ** Never close() a Unix fd that you haven't open()d or dup()d
   ** yourself.

Actually, one could formulate it more clearly:

   ** Never call close() unless you really know what you're doing.

That is easy to understand for everybody, I reckon. I think it is much
more likely that a programmer forgets a close() call than that he
erroneously writes one, especially because it is pretty hard to even
find the hidden dup() that creates all these fd's. When I was new to
wine I guess it took me least a week to understand why the close() calls
appeared everywhere.

The current rule is much more complex. Especially with asynchronous IO 
it becomes *very* hard to track the open fd's.

Please rethink your position on this subject.

Martin

-- 
Martin Wilck                Phone: [phone]
Fujitsu Siemens Computers   Fax:   [phone]
Heinz-Nixdorf-Ring 1	    mailto:Martin.Wilck(a)Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy

[Alexandre Julliard]

Martin Wilck <Martin.Wilck(a)Fujitsu-Siemens.com> writes:
> Am Fre, 2003-01-31 um 22.37 schrieb Alexandre Julliard:
> > I think it's cleaner to return a dup of the fd instead of relying on
> > the fact that the fd is cached internally. This may not always be the
> > case, and it's better to risk an fd leak than to risk invalidating the
> > cached value in case someone closes the fd when they shouldn't.
>
> I see your point, but I don't see the risk you're talking about.

The risk is a detail, the real point is that an API that returns an fd
to a client has to return a new fd and let the client close it, since
we can't know when the client is finished with it. Currently because
of the fd cache it happens that we don't really need the close() but
that's an implementation detail that could change; and we need to keep
the flexibility to change it.
> The current rule is much more complex. Especially with asynchronous IO 
> it becomes *very* hard to track the open fd's.

I don't see why it would be hard to close fds, it's just standard
resource management, like freeing allocated memory, etc.  If the async
I/O code makes this hard to do then the code is broken.

-- 
Alexandre Julliard
julliard(a)winehq.com

[Martin Wilck]

Am Don, 2003-02-06 um 22.17 schrieb Alexandre Julliard:
> The risk is a detail, the real point is that an API that returns an fd
> to a client has to return a new fd and let the client close it, since
> we can't know when the client is finished with it. Currently because
> of the fd cache it happens that we don't really need the close() but
> that's an implementation detail that could change; and we need to keep
> the flexibility to change it.

It is not clean to hide the system call that creates the fd and require
the client to make the call that destroys it. 

It'd be better to have the client call wine_server_get_fd() (as now)  to
get the fd and wine_server_release_fd() if it's done with it. Then it'd
be entirely up to wine core functionality whether or not
get_fd()/release_fd() requires a dup() and a close().

That'd be much better than what we have now, and keep the flexibility 
you're talking about. Instead of doing dup() and close(), we could have
a usage count associated with a Unix fd.

Can we come to an agreement along these lines?
> I don't see why it would be hard to close fds, it's just standard
> resource management, like freeing allocated memory, etc.  If the async
> I/O code makes this hard to do then the code is broken.

Asynchronous IO, broken or not, allows for hundreds of simultaneous IO
operations on a single HANDLE, and therefore gets us into *resource
shortage* as long as we use up an fd for each such operation.

Martin

-- 
Martin Wilck                Phone: [phone]
Fujitsu Siemens Computers   Fax:   [phone]
Heinz-Nixdorf-Ring 1	    mailto:Martin.Wilck(a)Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy

[Dan Kegel]

Martin Wilck wrote:
> > I don't see why it would be hard to close fds, it's just standard
> > resource management, like freeing allocated memory, etc.  If the async
> > I/O code makes this hard to do then the code is broken.
>
> Asynchronous IO, broken or not, allows for hundreds of simultaneous IO
> operations on a single HANDLE, and therefore gets us into *resource
> shortage* as long as we use up an fd for each such operation.

Is your main objection to the current scheme based on performance?
- Dan

-- 
Dan Kegel
http://www.kegel.com
http://counter.li.org/cgi-bin/runscript/display-person.cgi?user=78045

[Alexandre Julliard]

Martin Wilck <Martin.Wilck(a)Fujitsu-Siemens.com> writes:
> Asynchronous IO, broken or not, allows for hundreds of simultaneous IO
> operations on a single HANDLE, and therefore gets us into *resource
> shortage* as long as we use up an fd for each such operation.

That of course is a completely different issue, and I agree this one
needs solving. But it's not clear at all that we need to change the
generic functionality that works just fine in other cases. IMO we
should change the way the async I/O works to avoid wasting file
descriptors. This probably means some kind of reference counting, but
since this is only necessary for async I/O it should be done there,
not in the generic code.

-- 
Alexandre Julliard
julliard(a)winehq.com