You can use this patch as a basis for fixing on Linux / MacOSX. I've re-written the code in GDBRemoteCommunication.cpp already, so all you have to do is make PipePosix compile with these changes.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Actually this isn't right. It gets the compile working, but we need to
split out the notion of opening an existing pipe versus creating both the
read/write ends for a new pipe. I guess I need to revert the original
change until this gets sorted out.
Comment Actions
I'm thinking about having Pipe class as a base class with derived AnonymousPipe and NamedPipe.
AnonymousPipe will be almost replica of existing Pipe class with custom Open(OpenOptions opts) method.
NamedPipe will have following own methods:
enum OpenOptions
{
eOpenOptionNonBlocking = (1u << 0), // Non-blocking reads
eOpenChildProcessesInherit = (1u << 1) // Child processes inherit
};explicit NamedPipe(const char* name); bool Create(); //mkfifo bool Delete(); //Close() & unlink bool OpenReader(OpenOptions opts); bool OpenWriter(OpenOptions opts);
For named pipes we don't need to have both reader & writer opened in the same processes - so, that's why I think we may need separate open methods.
In order to read with timeout pipe should be opened in non-blocking mode - on Windows you may use PIPE_NOWAIT if such option is set.
Comment Actions
I thought about something like that too, but then I wondered if it's really
necessary. In Windows we need to open a named pipe internally even when
the user requests anonymous pipe, because a named pipe is the only way to
achieve something like select. So that would make that scenario really
awkward.
It seems like we can do the same thing as you've suggested but without the
inheritance.
Error Create(llvm::StringRef name); name is optional
Error OpenReader(llvm::StringRef name); name is required
Error OpenWriter(llvm::StringRef name); // name is required
Error Read(buf, size, bytes_read);
Error ReadWithTimeout(buf, size, timeout, bytes_read);
I would prefer not having to specify the non-blocking option. The reason
is that if user doesn't specify nonblocking, then ReadWithTimeout is an
invalid method to call. And if they do specify non-blocking, the
implementation of Read() changes.
But we can always support both methods if we always open the pipe
nonblocking. In read you just implement the block yourself even though the
pipe is opened with O_NONBLOCK
Comment Actions
It looks to me that separate classes for anonymous and named pipe make interface more clear.
There is no dual interpretation for some methods like IsValid(): for anonymous pipe it presumes both pipe handles are valid, in case of named pipe - it's okay to treat pipe object as valid if at least one (either read or write) handle is valid.
If there is a single pipe class it might be confusing for user which methods are applicable for anonymous or named pipes.
Windows implementation of anonymous pipe may create internally an instance of named pipe and delegate all calls to it.
It seems like we can do the same thing as you've suggested but without the
inheritance.Error Create(llvm::StringRef name); name is optional
Error OpenReader(llvm::StringRef name); name is required
Error OpenWriter(llvm::StringRef name); // name is required
I propose to make Create/OpenReader/OpenWriter/Delete take a pipe name as instance field (initialized in constructor) instead of input parameter. Otherwise It may mean that class allows to hold read and write handles from different pipes.
Error Read(buf, size, bytes_read);
Error ReadWithTimeout(buf, size, timeout, bytes_read);I would prefer not having to specify the non-blocking option. The reason
is that if user doesn't specify nonblocking, then ReadWithTimeout is an
invalid method to call. And if they do specify non-blocking, the
implementation of Read() changes.But we can always support both methods if we always open the pipe
nonblocking. In read you just implement the block yourself even though the
pipe is opened with O_NONBLOCK
Good point.
Comment Actions
Instead of IsValid() we could have CanRead() and CanWrite(). In the case
of someone calling Create(), both should return true. In the case of
someone calling OpenReader() then CanRead() returns true and CanWrite()
returns false. And simillarly for someone calling OpenWriter().
This way, all methods are always applicable for every type of pipe,
anonymous or named. Also, when calling any of the methods, the
implementation could close both the existing read fd (if it's valid) and
the existing write fd (if it's valid). This way there would be no way to
get a Pipe object with handles from two different pipes.
The reason I'm a little leary of multiple implementations is because Read /
Write is implemented identically in all three cases, and that's the primary
operation on a pipe. So the user doesn't shouldn't have to worry about
what type it is. Furthermore, it would require creating 8 new source files
(2 headers and 2 cpp files on 2 different platform), for a questionable
benefit. It seems like making separate classes for each would be similar
to making ReadWriteFile, ReadOnlyFile, and WriteOnlyFile. Ultimately the
only difference is going to be what flags you pass to the creation method.