[opensource-dev] Review Request: STORM-1112 Support SOCKS 5 proxy in the viewer (take 2)

[Richard Nelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/374/#review1005
-----------------------------------------------------------



indra/llcommon/llsingleton.h
<http://codereview.secondlife.com/r/374/#comment1018>

    why are we getting the data from within the destructor of the singleton itself?  Why not just do if (mInitState != DELETED) instead?  Seems like it would be less confusing.



indra/llmessage/llcurl.h
<http://codereview.secondlife.com/r/374/#comment1019>

    maybe a little comment as to what "Easy" is in this context.  I know you just moved the code to the header, but now that it is more than an implementation detail of LLCurl, I think our expectation of documentation has increased.



indra/llmessage/llcurl.cpp
<http://codereview.secondlife.com/r/374/#comment1020>

    if time_out is unused now, let's remove it from the signature



indra/llmessage/llproxy.h
<http://codereview.secondlife.com/r/374/#comment1021>

    do all these #defines and socks structs need to be in the header?



indra/llmessage/llproxy.h
<http://codereview.secondlife.com/r/374/#comment1022>

    I think we need something a bit more obvious to call out locking methods, maybe just a heavier comment header?



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment1025>

    control-flow wise, it might be clearer to move the final check for status to a separate if, with appropriate comment along the lines of "if any of the above failed..."



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment1026>

    can we reflect the potentially long blocking period in the name of the method?
    
    tcp_wait_for_handshake or similar?


- Richard


On Aug. 25, 2011, 11:15 a.m., Log Linden wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/374/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2011, 11:15 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/llcommon/llerror.h 4ebbd04efd93 
>   indra/llcommon/llerror.cpp 4ebbd04efd93 
>   indra/llcommon/llsingleton.h 4ebbd04efd93 
>   indra/llmessage/CMakeLists.txt 4ebbd04efd93 
>   indra/llmessage/llcurl.h 4ebbd04efd93 
>   indra/llmessage/llcurl.cpp 4ebbd04efd93 
>   indra/llmessage/llhttpassetstorage.cpp 4ebbd04efd93 
>   indra/llmessage/llhttpclient.cpp 4ebbd04efd93 
>   indra/llmessage/lliosocket.h 4ebbd04efd93 
>   indra/llmessage/lliosocket.cpp 4ebbd04efd93 
>   indra/llmessage/llpacketring.h 4ebbd04efd93 
>   indra/llmessage/llpacketring.cpp 4ebbd04efd93 
>   indra/llmessage/llproxy.h PRE-CREATION 
>   indra/llmessage/llproxy.cpp PRE-CREATION 
>   indra/llmessage/llurlrequest.cpp 4ebbd04efd93 
>   indra/llmessage/net.h 4ebbd04efd93 
>   indra/llmessage/net.cpp 4ebbd04efd93 
>   indra/llui/llfunctorregistry.h 4ebbd04efd93 
>   indra/newview/app_settings/settings.xml 4ebbd04efd93 
>   indra/newview/llappviewer.cpp 4ebbd04efd93 
>   indra/newview/llfloaterpreference.h 4ebbd04efd93 
>   indra/newview/llfloaterpreference.cpp 4ebbd04efd93 
>   indra/newview/llloginhandler.cpp 4ebbd04efd93 
>   indra/newview/llpanellogin.h 4ebbd04efd93 
>   indra/newview/llsecapi.h 4ebbd04efd93 
>   indra/newview/llstartup.h 4ebbd04efd93 
>   indra/newview/llstartup.cpp 4ebbd04efd93 
>   indra/newview/llviewerfloaterreg.cpp 4ebbd04efd93 
>   indra/newview/llxmlrpctransaction.cpp 4ebbd04efd93 
>   indra/newview/skins/default/xui/en/floater_preferences_proxy.xml PRE-CREATION 
>   indra/newview/skins/default/xui/en/notifications.xml 4ebbd04efd93 
>   indra/newview/skins/default/xui/en/panel_cof_wearables.xml 4ebbd04efd93 
>   indra/newview/skins/default/xui/en/panel_preferences_privacy.xml 4ebbd04efd93 
>   indra/newview/skins/default/xui/en/panel_preferences_setup.xml 4ebbd04efd93 
> 
> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110830/2579f4e5/attachment-0001.htm