Re: [Barry-devel] WinCE port of Barry

[Toby G. <tob...@re...>]

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