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 Martin.Wilck@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 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
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 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.
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
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
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
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?
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" Martin.Wilck@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.
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?
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
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 Martin.Wilck@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.
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 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
Martin Wilck Martin.Wilck@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 -
Dan Kegel -
David Laight -
Dmitry Timoshkov -
Martin Wilck -
Michael Stefaniuc -
Mike McCormack -
Shachar Shemesh