Re: [Barry-devel] WinCE port of Barry
Status: Beta
Brought to you by:
ndprojects
From: Chris F. <cd...@fo...> - 2012-10-05 01:35:14
|
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 |