On 05/10/12 02:35, Chris Frey wrote: > 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.
That's understandable. I know how easy it is for other work to get in the way. > > >> 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. :-) Good point. I've put a fix for this in my wince branch. > > 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. Another good point. Again I've put a fix for this in my wince branch. >> 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! It's a shame it is a bit complex, but at least it avoids the TransferInterest and redelivering packets. I imagine that this hasn't been seen before as none of the other socket mode protocols start with data coming from the device. >> 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! Thank you for reviewing and pulling my changes. Regards, Toby ------------------------------------------------------------------------------ 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
