[opensource-dev] Review Request: VWR-20801 Implement SOCKS 5 Proxy for the viewer

2011-03-28 Thread Robin Cornelius 

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

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

2011-03-28 Thread Philippe (Merov) Bossut 
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
wrote:

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

Re: [opensource-dev] Review Request: VWR-20801 Implement SOCKS 5 Proxy for the viewer

2011-03-29 Thread Merov Linden 

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


Could write that in 1 line to make it easier to read.



indra/llmessage/llsocks5.h


Incorrect license and header. Please use the same as for the rest of the 
code.



indra/llmessage/llsocks5.h


Typo: available



indra/llmessage/llsocks5.h


Typo: suppress final ";"



indra/llmessage/llsocks5.h


naming convention: we prefix private member names with "m", not "h"



indra/llmessage/llsocks5.cpp


Wrong license again



indra/llmessage/llsocks5.cpp


Typos: association, associate



indra/llmessage/llsocks5.cpp


Type: method



indra/llmessage/llsocks5.cpp


Typo: indentation incorrect



indra/llmessage/net.cpp


Typo: for



indra/llmessage/net.cpp


Could you rephrase that warning? I can't really understand what it means. 



indra/llmessage/net.cpp


Typo: connected



indra/newview/llfloaterpreference.cpp


Typos: it's (verb), until



indra/newview/llstartup.cpp


Typos: it's (verb)



indra/newview/llxmlrpctransaction.cpp


Typos: incorrect indentation



indra/newview/skins/default/xui/en/floater_preferences_proxy.xml


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 her

Re: [opensource-dev] Review Request: VWR-20801 Implement SOCKS 5 Proxy for the viewer

2011-03-29 Thread Monty Brandenberg 


> 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

2011-04-01 Thread Monty Brandenberg 

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


Wonder if the new GUI layout should have a review



indra/newview/llfloaterpreference.cpp


General whitespace comment applies.  I *think* we're still doing tabs, yes?



indra/newview/llfloaterpreference.cpp


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


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


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


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


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


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


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


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