Page MenuHomePhabricator

Extend PipePosix with support for named pipes/timeout-based IO and integrate it with GDBRemoteCommunication / lldb-gdbserver.
ClosedPublic

Authored by ovyalov on Jan 13 2015, 12:08 PM.

Details

Reviewers
zturner
clayborg
Summary

Make PipePosix to support next operations:

  • Create a named pipe.
  • Open reader and writer sides of a named pipe.
  • Timeout-based Read and Write operations.
  • Added Delete call to PipeBase interface since named pipe removal differs for POSIX and Windows pipes.

Integrated Pipe class into GDBRemoteCommunication::StartDebugserverProcess and lldb-gdbserver in order to support reading and writing a port value to a named pipe.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 18102.Jan 13 2015, 12:08 PM
ovyalov retitled this revision from to Extend PipePosix with support for named pipes/timeout-based IO and integrate it with GDBRemoteCommunication / lldb-gdbserver. .
ovyalov updated this object.
ovyalov edited the test plan for this revision. (Show Details)
ovyalov added reviewers: zturner, clayborg.
ovyalov added a subscriber: Unknown Object (MLST).
clayborg accepted this revision.Jan 13 2015, 12:37 PM
clayborg edited edge metadata.

We haven't been using std::chrono in any API yet, but it would be nice to convert over to using it more as fixes permit.

Looks fine.

This revision is now accepted and ready to land.Jan 13 2015, 12:37 PM
zturner added inline comments.Jan 13 2015, 12:53 PM
source/Host/posix/PipePosix.cpp
186

Why did we change this from OpenAsReader to OpenAsReaderWithTimeout? The timeout is never used, for one thing, so it's buggy, and it's less flexible, since you can't specify the timeout on individual reads.

zturner requested changes to this revision.Jan 13 2015, 12:53 PM
zturner edited edge metadata.
This revision now requires changes to proceed.Jan 13 2015, 12:53 PM
ovyalov updated this revision to Diff 18113.Jan 13 2015, 2:17 PM
ovyalov edited edge metadata.

My initial intent was to make interface of OpenAsReader symmetrical to what we have for OpenAsWriter. But since timeout isn't used by OpenAsReaderWithTimeout I've removed this method.
Please take another look!

Thank you.

Looks good, I like this better. In the future if we need to write with timeout we can add a WriteWithTimeout(). But usually that's not a common operation since it's not often you have to serialize on writes, so I expect it won't be needed.

Feel free to submit after removing OpenAsWriterWithTimeout().

include/lldb/Host/PipeBase.h
30

This probably needs to go away now too.

zturner accepted this revision.Jan 13 2015, 2:26 PM
zturner edited edge metadata.
This revision is now accepted and ready to land.Jan 13 2015, 2:26 PM

Please see my comment.

include/lldb/Host/PipeBase.h
30

We need this method on POSIX systems since attempt to open a writer in non-blocking mode fails with ENXIO if corresponding reader side hasn't opened yet. So, timeout allows to wait for a reader to be opened.

zturner added inline comments.Jan 13 2015, 2:46 PM
include/lldb/Host/PipeBase.h
30

Couldn't we implement the timeout at a higher level then? It's confusing currently, because in ReadWithTimeout(), a timeout value of 0 means block indefinitely, and in OpenAsWriterWithTimeout(), it means just return an error.

Maybe OpenAsWriter() can simply return an error and you can implement the retry at a higher level? It could return an error from the generic category indicating that the reader hasn't opened yet, and then Windows can do the same, so that the retry logic could be written at a higher level.

Reading and writing are a little different in that with a Read, the most common use case is to block until its finished, and you very rarely want to return immediately and do something else if there's no data. So it makes sense for ReadWithTimeout() to use 0 == INFINITE, but then OpenForWriteWithTiemout() becomes confusing.

Thoguhts?

ovyalov added inline comments.Jan 13 2015, 2:56 PM
include/lldb/Host/PipeBase.h
30

I believe both ReadWithTimeout and OpenAsWriterWithTimeout follow the same pattern - if timeout == zero() a operation blocks until it either succeeds or fails.
OpenAsWriterWithTimeout with zero timeout retries infinitely (sleeping 0.1 sec between retries) until writer successfully opened or a permanent error encountered (i.e. any error != ENXIO) - e.g. invalid pipe name.

Ahh, you're right, misread the code. No complaints then.

Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r225849.