Bill Medland wrote:
On April 14, 2003 10:34 pm, Dan Kegel wrote:
In fact, here's the smallest program that reproduces the problem for me:
#include <windows.h>
main() { FlushFileBuffers(CreateNamedPipeA("\\.\PiPe\foo", PIPE_ACCESS_DUPLEX, PIPE_TYPE_BYTE|PIPE_WAIT, 1, 1024, 1024, NMPWAIT_USE_DEFAULT_WAIT, NULL)); }
But that is not what I am fixing.
All I was saying is that the above is a minimal test case for the problem your patch fixes. Without your patch or something like it, wineserver crashes for me.
/*
- namedp/server.c
- Testbed server to find out what error is returned when reading from a pipe
- that has been broken by the server closing it.
*/ h = CreateNamedPipe ("\\.\pipe\tstpipe", PIPE_ACCESS_DUPLEX, PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, 1, 512, 512, 60000, NULL)) ConnectNamedPipe (h, NULL) ReadFile (h, &req, sizeof(req), &num_read, NULL) WriteFile (h, &reply, sizeof(reply), &num_written, NULL) /* Deliberately do not flush; this is what we want to test */ DisconnectNamedPipe (h); CloseHandle (h);
/*
- namedp/client.c
- Testbed client to find out what error is returned when reading from a pipe
- that has been broken by the server closing it.
*/ h = CreateFile ("\\.\pipe\tstpipe", GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING, 0, NULL)) WriteFile (h, &req, sizeof(req), &num_written, NULL)
/* This is where we want to hang around for the server to complete */ Sleep (20000); if (!ReadFile (h, &reply, sizeof(reply), &num_read, NULL) || num_read !=sizeof (reply)) { fprintf (stderr, "Error %lx or incomplete %d cf %d on ReadFile\n", GetLastError(), num_read, sizeof (reply)); CloseHandle (h); return 1; } CloseHandle (h); fprintf (stdout, "Reply was %lu\n", reply); return 0; }
Thanks for posting the code. So you're checking to make sure that data doesn't get lost on a DisconnectNamedPipe?
The point, as far as I am concerned, is that there is an error in the use of the "get object's fd" protocol. The protocol has to handle three different situations:
- The request is being made of an object that doesn't have fds; the request
should never have been made in the first place. (For example a process). Note that no_get_fd() already sets the last error so there is no need for get_handle_fd_obj to do so. 2. The request is being made of an object that does have fds and at the time of the request it actually has one. That is reasonable; each object knows where it stored it and its "get fd" function can return it. 3. The request is being made of an object that does have fds in general, but doesn't actually have one at this moment. That was the problem in the named pipe code.
Now clearly (since the error "ERROR_PIPE_NOT_CONNECTED" (or whatever it was) is pipe-specific) we must leave it to the object to set the error, not to the general fd code. (I would hate to see code in fd.c that actually knew what sort of object it was looking at). That is my justification for the fix that I submitted;
I agree in general, but looking at your patch, it appears to cause FlushFileBuffers to return an error when the object has no fds. At least in my late night quick test on win2K, it's quite ok to call FlushFileBuffers on an object that in general have fds but doesn't have one at the moment.
It may take some rejiggering to fix this without putting pipe-specific code where it doesn't belong. At the moment, I think you can't call the object's flush routine until you have an fd, which means the object doesn't get a chance to say "Oh, ok, it's a flush request when I don't have an fd, no error".
But then as I've said earlier, I hardly understand this code, so I should just keep quiet :-)
I presume other people (Mike especially) are already doing the flushing code, which clearly has to be quite specialised since it is blocking on the client's reading of the pipe. Clearly that code will have to handle flushing a handle that does not yet have a fd on it. (I haven't looked but I presume that the fd gets added when the pipe is connected).
Mike isn't doing flushing code for this version of the named pipe implementation, as it's not clear it's possible without a rewrite. He's replacing the whole thing. - Dan