On Thu, Sep 27, 2012 at 04:55:19PM +0100, Toby Gray wrote: > Hi, > > I've finally managed to get enough spare timeto make the changes that > Chris suggested in his review comments. I've updated my wince branch in > github: > https://github.com/tobygray/barry/tree/wince
Thanks Toby! My time for Barry has been limited lately, but I took a brief look tonight. > The good news is that I managed to get a solution to the tr1 problem > without needing to change the barry headers: > https://github.com/tobygray/barry/commit/af3d23b39279930f755bfb8a0aac5d91e5f7aec3 Nice. Regarding the new TCP stream support: I notice that in the unix implementation for brawchannel, you use both ::read() and fwrite() in the Stdin/Stdout stream classes. Is there a reason for this? The ::read() is straight, unix, unbuffered I/O, while the fwrite() is buffered. For buffered streams, we normally use cout. It is safe to use fwrite() here, since we have cout.sync_with_stdio(true); in most of the tools as I recall, but it would be nice if Barry code didn't have this dependency. I think ::write() is satisfactory, unless you know something I don't. :-) I've merged this, since this is strictly tools/ code, but I can see stream classes like this being useful elsewhere, and could in the future be moved to a library, so it would be good to change this. Also... +TcpStream::TcpStream(const char * addr, long port) +{ + mImpl.reset(new TcpStreamImpl); + mImpl->mListenAddress = addr; + mImpl->mPort = port; + mImpl->mSocket = INVALID_SOCKET; + mImpl->mListenSocket = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP); + if( mImpl->mListenSocket == INVALID_SOCKET ) { + cerr << "Failed to create listening socket: " << + errno << endl; + } + if( mImpl->mListenAddress == NULL ) { + mImpl->mHostAddress.s_addr = INADDR_ANY; + } else { + mImpl->mHostAddress.s_addr = inet_addr(mImpl->mListenAddress); + } +} This takes an external const char* and saves it internally, for later use. This is also dangerous in code that could be used in the library. I'd use a std::string in the TcpStreamImpl struct here for safety. > I also fixed the registering for data packet handling without needing > the messy transfer interest function that my previous patches had > implemented. This is better than TransferInterest(). Clever too. I didn't like exposing RegisterInterest() with a type argument as the default option, so I modified things a bit and added comments, trying to make it clear that most users shouldn't use the new API. Socket::SyncSend() really needs both data and sequence packets in order to function, so this entire API is really only for the socket opening race condition. It still seems a bit heavy to me, but I don't have a better way in mind, and this is pretty clean as is! :-) Thanks! > I based all these changes off the v0.19.0 branch rather than master as I > that seemed to make more sense. I've rebased the v0.19.0 branch onto master, and have included all your work. Thanks again! - Chris ------------------------------------------------------------------------------ Don't let slow site performance ruin your business. Deploy New Relic APM Deploy New Relic app performance management and know exactly what is happening inside your Ruby, Python, PHP, Java, and .NET app Try New Relic at no cost today and get our sweet Data Nerd shirt too! http://p.sf.net/sfu/newrelic-dev2dev _______________________________________________ Barry-devel mailing list Barry-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/barry-devel
