----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/374/#review863 -----------------------------------------------------------
indra/llmessage/llcurl.cpp <http://codereview.secondlife.com/r/374/#comment880> Correct whitespace. indra/llmessage/llpacketring.h <http://codereview.secondlife.com/r/374/#comment884> A few things mostly on coding style here: 1. You'll mostly see method declarations separate from member declarations. It's often due to access controls but it's also a good organizational thing. I'd move the method up here. 2. This is really a private/protected method. No external user should call this. Making it so is compatible with 1. 3. Terrible name. SendPacket and doSendPacket. This is some sort of sendPacketImpl or routePacket, I think. indra/llmessage/llpacketring.cpp <http://codereview.secondlife.com/r/374/#comment890> With the proxy code, we could get data larger than NET_BUFFER_SIZE. We don't detect this condition in receive_packet. Might just log initially. So much wrong with it.... indra/llmessage/llpacketring.cpp <http://codereview.secondlife.com/r/374/#comment886> Magic number from original code review. Need these definitions (10) in the socks protocol definition set. indra/llmessage/llpacketring.cpp <http://codereview.secondlife.com/r/374/#comment888> Not necessary now but something for a follow-on jira. This would have been a good place for a scatter/gather i/o using recvmsg to avoid the data copying. indra/llmessage/llpacketring.cpp <http://codereview.secondlife.com/r/374/#comment891> Isn't packet_size '10' too large at this point? indra/llmessage/llpacketring.cpp <http://codereview.secondlife.com/r/374/#comment889> I just looked into get_sender() and get_receiving_interface(). *weep* They use statics. So no thread safety. If we do improve the receive_packet interface to ever to scatter/gather, *this* will go. indra/llmessage/llpacketring.cpp <http://codereview.secondlife.com/r/374/#comment892> Another perfect opportunity for scatter/gather i/o. indra/llmessage/llpacketring.cpp <http://codereview.secondlife.com/r/374/#comment893> Caller may still reasonably try to write a full NET_BUFFER_SIZE of data in which case this smashes memory. - Monty On July 12, 2011, 11:20 a.m., Log Linden wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/374/ > ----------------------------------------------------------- > > (Updated July 12, 2011, 11:20 a.m.) > > > Review request for Viewer, Oz Linden, Monty Brandenberg, and Stone Linden. > > > Summary > ------- > > This is a continuation of Robin Cornelius's SOCKS 5 contribution, shown in > https://codereview.secondlife.com/r/232/ . I have tried to address all of > the comments on that code review and do as much cleanup as possible. The diff > includes everything that was submitted by Robin, as well as my work. > Major changes since I started working: > * Changed SOCKS 5 proxy control channel to use the existing LLSocket class, > which is a thin wrapper around APR sockets. > * Worked with the Linden Lab UX team to revamp the proxy controls. > * Proxy credentials are now stored in the LLSecAPI password storage, which is > the same that is used for users' Second Life Credentials instead of as being > stored in the clear as a preference. > > > This addresses bug STORM-1112. > http://jira.secondlife.com/browse/STORM-1112 > > > Diffs > ----- > > indra/llmessage/CMakeLists.txt c7a4b7a24e05 > indra/llmessage/llcurl.cpp c7a4b7a24e05 > indra/llmessage/lliosocket.h c7a4b7a24e05 > indra/llmessage/lliosocket.cpp c7a4b7a24e05 > indra/llmessage/llpacketring.h c7a4b7a24e05 > indra/llmessage/llpacketring.cpp c7a4b7a24e05 > indra/llmessage/llproxy.h PRE-CREATION > indra/llmessage/llproxy.cpp PRE-CREATION > indra/llmessage/net.h c7a4b7a24e05 > indra/llmessage/net.cpp c7a4b7a24e05 > indra/llui/llfunctorregistry.h c7a4b7a24e05 > indra/newview/app_settings/settings.xml c7a4b7a24e05 > indra/newview/llappviewer.cpp c7a4b7a24e05 > indra/newview/llfloaterpreference.h c7a4b7a24e05 > indra/newview/llfloaterpreference.cpp c7a4b7a24e05 > indra/newview/llloginhandler.cpp c7a4b7a24e05 > indra/newview/llpanellogin.h c7a4b7a24e05 > indra/newview/llsecapi.h c7a4b7a24e05 > indra/newview/llstartup.h c7a4b7a24e05 > indra/newview/llstartup.cpp c7a4b7a24e05 > indra/newview/llviewerfloaterreg.cpp c7a4b7a24e05 > indra/newview/llxmlrpctransaction.cpp c7a4b7a24e05 > indra/newview/skins/default/xui/en/floater_preferences_proxy.xml > PRE-CREATION > indra/newview/skins/default/xui/en/notifications.xml c7a4b7a24e05 > indra/newview/skins/default/xui/en/panel_cof_wearables.xml c7a4b7a24e05 > indra/newview/skins/default/xui/en/panel_preferences_privacy.xml > c7a4b7a24e05 > indra/newview/skins/default/xui/en/panel_preferences_setup.xml c7a4b7a24e05 > > Diff: http://codereview.secondlife.com/r/374/diff > > > Testing > ------- > > I've tested exclusively on Linux so far. I'm working on a more extensive > test plan that includes setting up a gateway with a restrictive firewall to > verify that all traffic is going through the proxy. > > Test builds and screenshots of the changed UI elements are available from the > project page, located here: > https://wiki.secondlife.com/wiki/User:Log_Linden/Socks5Viewer > > > Thanks, > > Log > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
