On 03/09/2010 23:31, Chris Frey wrote:
> Hi Toby,
>
> Thanks again very much for your work on this. I have rebased your branch
> on to the latest master and pushed it out to the repos with some changes
> of my own. I rebase to make it easier for me to keep the CVS repositories
> on sourceforge up to date. Fortunately, there were no conflicts when
> I rebased your work.
Thanks for doing that. I noticed you made several style changes,
especially to where * and & go on types; I'll try to follow those in future.
In attempting to rebase to the latest master I managed to get terribly
confused and so have just set up a new fork of the barry code at:
github.com/tobygray/barry
> I do have some questions and issues that need to be addressed before
> releasing 0.17, though.
Sure.
> +void RawChannel::OnOpen()
> +{
> + // Enable sequence packets so that DataSendAck callback and close can
> be
> + // implemented
> + m_zero_registered = true;
> + m_socket->HideSequencePacket(false);
> + std::tr1::shared_ptr<SocketRoutingQueue::SocketDataHandler> callback;
> + callback.reset(new RawChannelSocketHandler(*this));
> + m_con.m_queue->RegisterInterest(0, callback);
> + // Get socket data packets routed to this class as well if using
> callback
> + // otherside just request interest
> + if( !m_callback ) {
> + // Don't want to be called back immediately on data
> + callback.reset();
> + }
> + m_socket->RegisterInterest(callback);
> +}
>
> Why are you registering an interest with a potentially empty callback?
> Are you trying to throw away data? Or are you assuming the socket class
> will see the empty callback and ignore registering it?
A null callback is deliberately registered so that the router will place
messages for that socket into a per socket queue for later reading with
SocketRoutingQueue::SocketRead. I'll clarify the comment to make that
clearer.
> --- a/src/socket.cc
> +++ b/src/socket.cc
> @@ -501,6 +501,12 @@ SocketHandle SocketZero::Open(uint16_t socket, const
> char *
> password)
> // fall through to success check...
> }
>
> + if( packet.Command() == SB_COMMAND_CLOSE_SOCKET )
> + {
> + eout("Packet:\n"<< receive);
> + throw Error("Socket: Socket closed when trying to open");
> + }
> +
> if( packet.Command() != SB_COMMAND_OPENED_SOCKET ||
> packet.SocketResponse() != socket ||
> packet.SocketSequence() != closeFlag )
> @@ -566,9 +572,15 @@ void SocketZero::Close(Socket&socket)
> {
> // reset so this won't be called again
> socket.ForceClosed();
> -
> - eout("Packet:\n"<< response);
> - throw BadPacket(rpack->command, "Socket: Bad CLOSED packet in
> Close");
> +
> + if( rpack->command == SB_COMMAND_REMOTE_CLOSE_SOCKET ) {
> + eout("Remote end closed connection");
> + throw BadPacket(rpack->command, "Socket: Remote close
> packet in Close");
> + }
> + else {
> + eout("Packet:\n"<< response);
> + throw BadPacket(rpack->command, "Socket: Bad CLOSED
> packet in Close");
> + }
> }
>
>
> Can you explain the logic here?
Sure. I'll update the comments as well.
> How can CLOSE_SOCKET be returned when
> we just sent the password handshake?
If the BlackBerry is in a state where it thinks the connection is open
then it seems to send a SB_COMMAND_CLOSE_SOCKET message when attempting
to open that socket. The easiest way to get the BlackBerry into a state
where it thinks the socket is open but it isn't is to kill the
brawchannel process with SIGKILL while a connection is established.
> And are all these errors worthy
> of exceptions?
Yes and no. I think an exception when receiving SB_COMMAND_CLOSE_SOCKET
when trying to open the socket makes sense. However I agree that it
doesn't really make sense to throw an exception when closing if the
BlackBerry has closed the channel. I'll update that code to not throw an
exception.
> I assume this code has something to do with the new raw channel protocol,
> and I just want to better understand it.
Yes, as far as I know, it's only needed for the channel support.
> In m_raw_channel.cc:
>
> +#define RAW_HEADER_SIZE 4
>
> That's a bug... this should go in src/protostructs.h as a struct.
> You don't want to rely on MIN_PACKET_DATA_SIZE to always be correct in
> src/protostructs.h (see m_raw_channel.cc line 164) when there are
> header size constants elsewhere in the code. Header sizes and definitions
> need to be defined in protostructs.h and used via #define or sizeof()
> elsewhere in the code.
>
> If you find places where I've also violated this rule, please let me know,
> since that's a bug too. :-) It's best to keep low level protocol information
> in one place.
>
> Actually, in RawChannel::Send(), you simply use struct Packet, filling
> in the first 4 bytes with socket and size information. If this is all
> that RAW_HEADER_SIZE represents, then you can remove it, and use
> SB_JVMPACKET_HEADER_SIZE, or you can create your own SB_RAWPACKET_HEADER_SIZE
> along with a struct for it, similar to struct JVMPacket and struct Packet.
I think creating a new struct and defines in the way to go. I don't know
why I didn't do that to start with, it'll certainly make the code far
neater.
> Similarly, RawChannel::MaximumSendSize() should just return a #define
> from protostructs.h. The low level packet size and structure information
> is stored in that header as much as possible.
Agreed.
> In router.h:
>
> There's a new flag, m_seen_usb_error, and it needed a way to reset it,
> and be an easy way to deal with USB errors.
>
> I figured that the best place to recover from USB errors is in the
> application code that calls Router::SetUsbDevice(), so I added a
> simple callback there, and a call to clear the flag in case of
> recovery.
Looks good. I'd not considered that API users would recover from USB
errors by reopening the device so thanks for fixing that.
Regards,
Toby
------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:
Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd
_______________________________________________
Barry-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/barry-devel
