Re: [opensource-dev] Review Request: VWR-20801 Implement SOCKS 5 Proxy for the viewer
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/232/#review537 --- Final pass through the code for review (I'll do this again after changes in the code). Two followups that must happen from SNOWSTORM: 1. Some panel/GUI changes that probably need review. 2. Password storage in the panel almost certainly needs to be moved to the LLSecAPI lump (or wherever 'Remember password' gets stuffed). indra/newview/llfloaterpreference.h http://codereview.secondlife.com/r/232/#comment458 Wonder if the new GUI layout should have a review indra/newview/llfloaterpreference.cpp http://codereview.secondlife.com/r/232/#comment459 General whitespace comment applies. I *think* we're still doing tabs, yes? indra/newview/llfloaterpreference.cpp http://codereview.secondlife.com/r/232/#comment460 We test mSocksSettingsDirty twice in this method unnecessarily (any differently). 'untill' - 'until' Re: comment. What are we telling the user here? It looks like we might not even save the settings. Explain. indra/newview/llfloaterpreference.cpp http://codereview.secondlife.com/r/232/#comment461 I have a suspicion here that we are saving the socks5 password to the generic settings mechanism. I don't think we can do that and we must use the LLSecAPI junk. indra/newview/llstartup.cpp http://codereview.secondlife.com/r/232/#comment462 It looks like this was disabled when it needed to be moved to some location after the proxy setup. Is that correct? I didn't see the new location in the code review tool... indra/newview/llstartup.cpp http://codereview.secondlife.com/r/232/#comment465 handleSocksProxy() seems to be setting up both standard HTTP proxies and Socks5 proxies. But this code is only allowing that for Socks5 proxies. Bad in itself but is caused by logic being replicated in several levels. This often indicates a design problem. indra/newview/llstartup.cpp http://codereview.secondlife.com/r/232/#comment463 There's a minor consistency problem in the settings checking here. If BrowserProxyEnabled and Socks5ProxyEnabled both are set true (accidentally, file corruption, logic error, whatever), one piece of code will treat this as having the BrowserProxyEnabled, another piece will treat it as Socks5ProxyEnabled. When I do these things, I like to sample the settings values once, normalize them to valid states and then code to those normalized values. indra/newview/llstartup.cpp http://codereview.secondlife.com/r/232/#comment464 Only variant in all these case statements is the first argument of the call. Might just set that in a stack variable and use it in one place after the switch. It is soo close to being stringizable. indra/newview/llxmlrpctransaction.cpp http://codereview.secondlife.com/r/232/#comment466 It's kind of clear that the 'LLSocks' class is really misnamed. It encompasses standard HTTP proxying as well as Socks5 proxying and if other styles come along, those as well. A class-and-file rename looks to be in order here. indra/newview/llxmlrpctransaction.cpp http://codereview.secondlife.com/r/232/#comment467 This piece of logic inside the isHttpProxyEnabled() test is beginning to look like a common idiom. Is it something that might be extracted into a free function in the LLSocks.cpp module to provide common glue between proxy configurations and libcurl option settings? - Monty On March 28, 2011, 4:46 a.m., Robin Cornelius wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/232/ --- (Updated March 28, 2011, 4:46 a.m.) Review request for Viewer. Summary --- VWR-20801 - Add ability to use SOCKS 5 proxy to the viewer. This allows the UDP and/or the http requests to be sent via a SOCKS 5 proxy. This also allows http proxies to be used for other http operations such as caps etc as required. All the proxy settings have been unified on a single proxy floater accessable from preferences. This addresses bug VWR-20801. http://jira.secondlife.com/browse/VWR-20801 Diffs - indra/llmessage/CMakeLists.txt 65ff7415f171 indra/llmessage/llcurl.cpp 65ff7415f171 indra/llmessage/llpacketring.h 65ff7415f171 indra/llmessage/llpacketring.cpp 65ff7415f171 indra/llmessage/llsocks5.h PRE-CREATION indra/llmessage/llsocks5.cpp PRE-CREATION indra/llmessage/net.h 65ff7415f171 indra/llmessage/net.cpp 65ff7415f171 indra/newview/app_settings/settings.xml 65ff7415f171 indra/newview/llfloaterpreference.h 65ff7415f171
Re: [opensource-dev] Review Request: VWR-20801 Implement SOCKS 5 Proxy for the viewer
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/232/#review519 --- Excellent! Except for a handful of minor typos, I've no problem with that code. One thing important though before we merge is to use the correct lgpl header for the new files. I hope others will also review and try it out before we merge as it's a fair amount of code. indra/llmessage/llpacketring.cpp http://codereview.secondlife.com/r/232/#comment398 Could write that in 1 line to make it easier to read. indra/llmessage/llsocks5.h http://codereview.secondlife.com/r/232/#comment396 Incorrect license and header. Please use the same as for the rest of the code. indra/llmessage/llsocks5.h http://codereview.secondlife.com/r/232/#comment399 Typo: available indra/llmessage/llsocks5.h http://codereview.secondlife.com/r/232/#comment400 Typo: suppress final ; indra/llmessage/llsocks5.h http://codereview.secondlife.com/r/232/#comment401 naming convention: we prefix private member names with m, not h indra/llmessage/llsocks5.cpp http://codereview.secondlife.com/r/232/#comment397 Wrong license again indra/llmessage/llsocks5.cpp http://codereview.secondlife.com/r/232/#comment402 Typos: association, associate indra/llmessage/llsocks5.cpp http://codereview.secondlife.com/r/232/#comment403 Type: method indra/llmessage/llsocks5.cpp http://codereview.secondlife.com/r/232/#comment404 Typo: indentation incorrect indra/llmessage/net.cpp http://codereview.secondlife.com/r/232/#comment405 Typo: for indra/llmessage/net.cpp http://codereview.secondlife.com/r/232/#comment406 Could you rephrase that warning? I can't really understand what it means. indra/llmessage/net.cpp http://codereview.secondlife.com/r/232/#comment407 Typo: connected indra/newview/llfloaterpreference.cpp http://codereview.secondlife.com/r/232/#comment408 Typos: it's (verb), until indra/newview/llstartup.cpp http://codereview.secondlife.com/r/232/#comment409 Typos: it's (verb) indra/newview/llxmlrpctransaction.cpp http://codereview.secondlife.com/r/232/#comment410 Typos: incorrect indentation indra/newview/skins/default/xui/en/floater_preferences_proxy.xml http://codereview.secondlife.com/r/232/#comment411 Typo: traffic - Merov On March 28, 2011, 4:46 a.m., Robin Cornelius wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/232/ --- (Updated March 28, 2011, 4:46 a.m.) Review request for Viewer. Summary --- VWR-20801 - Add ability to use SOCKS 5 proxy to the viewer. This allows the UDP and/or the http requests to be sent via a SOCKS 5 proxy. This also allows http proxies to be used for other http operations such as caps etc as required. All the proxy settings have been unified on a single proxy floater accessable from preferences. This addresses bug VWR-20801. http://jira.secondlife.com/browse/VWR-20801 Diffs - indra/llmessage/CMakeLists.txt 65ff7415f171 indra/llmessage/llcurl.cpp 65ff7415f171 indra/llmessage/llpacketring.h 65ff7415f171 indra/llmessage/llpacketring.cpp 65ff7415f171 indra/llmessage/llsocks5.h PRE-CREATION indra/llmessage/llsocks5.cpp PRE-CREATION indra/llmessage/net.h 65ff7415f171 indra/llmessage/net.cpp 65ff7415f171 indra/newview/app_settings/settings.xml 65ff7415f171 indra/newview/llfloaterpreference.h 65ff7415f171 indra/newview/llfloaterpreference.cpp 65ff7415f171 indra/newview/llstartup.h 65ff7415f171 indra/newview/llstartup.cpp 65ff7415f171 indra/newview/llviewerfloaterreg.cpp 65ff7415f171 indra/newview/llxmlrpctransaction.cpp 65ff7415f171 indra/newview/skins/default/xui/en/floater_preferences_proxy.xml PRE-CREATION indra/newview/skins/default/xui/en/notifications.xml 65ff7415f171 indra/newview/skins/default/xui/en/panel_preferences_setup.xml 65ff7415f171 Diff: http://codereview.secondlife.com/r/232/diff Testing --- Verified login and in world interaction with proxy disabled, verified login and in world interactionvia socks 5 proxy. Code has been tested on Windows very recently and has also worked fine on linux, but i'm not currently in a position to retest that or Mac at all. Much more testing is needed to verify this does not break anything unexpectedly and also works as expected when enabled. To test requires a working socks 5 proxy. Thanks, Robin ___ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep
Re: [opensource-dev] Review Request: VWR-20801 Implement SOCKS 5 Proxy for the viewer
On March 29, 2011, 4:19 p.m., Merov Linden wrote: Excellent! Except for a handful of minor typos, I've no problem with that code. One thing important though before we merge is to use the correct lgpl header for the new files. I hope others will also review and try it out before we merge as it's a fair amount of code. I definitely would like to review this but I can't for several days. But it's on my list. :-) - Monty --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/232/#review519 --- On March 28, 2011, 4:46 a.m., Robin Cornelius wrote: --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/232/ --- (Updated March 28, 2011, 4:46 a.m.) Review request for Viewer. Summary --- VWR-20801 - Add ability to use SOCKS 5 proxy to the viewer. This allows the UDP and/or the http requests to be sent via a SOCKS 5 proxy. This also allows http proxies to be used for other http operations such as caps etc as required. All the proxy settings have been unified on a single proxy floater accessable from preferences. This addresses bug VWR-20801. http://jira.secondlife.com/browse/VWR-20801 Diffs - indra/llmessage/CMakeLists.txt 65ff7415f171 indra/llmessage/llcurl.cpp 65ff7415f171 indra/llmessage/llpacketring.h 65ff7415f171 indra/llmessage/llpacketring.cpp 65ff7415f171 indra/llmessage/llsocks5.h PRE-CREATION indra/llmessage/llsocks5.cpp PRE-CREATION indra/llmessage/net.h 65ff7415f171 indra/llmessage/net.cpp 65ff7415f171 indra/newview/app_settings/settings.xml 65ff7415f171 indra/newview/llfloaterpreference.h 65ff7415f171 indra/newview/llfloaterpreference.cpp 65ff7415f171 indra/newview/llstartup.h 65ff7415f171 indra/newview/llstartup.cpp 65ff7415f171 indra/newview/llviewerfloaterreg.cpp 65ff7415f171 indra/newview/llxmlrpctransaction.cpp 65ff7415f171 indra/newview/skins/default/xui/en/floater_preferences_proxy.xml PRE-CREATION indra/newview/skins/default/xui/en/notifications.xml 65ff7415f171 indra/newview/skins/default/xui/en/panel_preferences_setup.xml 65ff7415f171 Diff: http://codereview.secondlife.com/r/232/diff Testing --- Verified login and in world interaction with proxy disabled, verified login and in world interactionvia socks 5 proxy. Code has been tested on Windows very recently and has also worked fine on linux, but i'm not currently in a position to retest that or Mac at all. Much more testing is needed to verify this does not break anything unexpectedly and also works as expected when enabled. To test requires a working socks 5 proxy. Thanks, Robin ___ 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
Re: [opensource-dev] Review Request: VWR-20801 Implement SOCKS 5 Proxy for the viewer
Hi Robin, Fantastic! I just went through the code and I'm not done reviewing and putting my Ship it! stamp on it but I wanted to thank you for completing this massive task. Thanks a lot! Cheers, - Merov On Mon, Mar 28, 2011 at 4:46 AM, Robin Cornelius robin.cornel...@gmail.comwrote: This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/232/ Review request for Viewer. By Robin Cornelius. Description VWR-20801 - Add ability to use SOCKS 5 proxy to the viewer. This allows the UDP and/or the http requests to be sent via a SOCKS 5 proxy. This also allows http proxies to be used for other http operations such as caps etc as required. All the proxy settings have been unified on a single proxy floater accessable from preferences. Testing Verified login and in world interaction with proxy disabled, verified login and in world interactionvia socks 5 proxy. Code has been tested on Windows very recently and has also worked fine on linux, but i'm not currently in a position to retest that or Mac at all. Much more testing is needed to verify this does not break anything unexpectedly and also works as expected when enabled. To test requires a working socks 5 proxy. *Bugs: * VWR-20801 http://jira.secondlife.com/browse/VWR-20801 Diffs - indra/llmessage/CMakeLists.txt (65ff7415f171) - indra/llmessage/llcurl.cpp (65ff7415f171) - indra/llmessage/llpacketring.h (65ff7415f171) - indra/llmessage/llpacketring.cpp (65ff7415f171) - indra/llmessage/llsocks5.h (PRE-CREATION) - indra/llmessage/llsocks5.cpp (PRE-CREATION) - indra/llmessage/net.h (65ff7415f171) - indra/llmessage/net.cpp (65ff7415f171) - indra/newview/app_settings/settings.xml (65ff7415f171) - indra/newview/llfloaterpreference.h (65ff7415f171) - indra/newview/llfloaterpreference.cpp (65ff7415f171) - indra/newview/llstartup.h (65ff7415f171) - indra/newview/llstartup.cpp (65ff7415f171) - indra/newview/llviewerfloaterreg.cpp (65ff7415f171) - indra/newview/llxmlrpctransaction.cpp (65ff7415f171) - indra/newview/skins/default/xui/en/floater_preferences_proxy.xml (PRE-CREATION) - indra/newview/skins/default/xui/en/notifications.xml (65ff7415f171) - indra/newview/skins/default/xui/en/panel_preferences_setup.xml (65ff7415f171) View Diff http://codereview.secondlife.com/r/232/diff/ ___ 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 ___ 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
