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

[Monty Brandenberg]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110712/1c6c27eb/attachment-0001.htm