Re: [tor-bugs] #2091 [Vidalia]: Support ControlSocket as well as ControlPort

["Tor Bug Tracker & Wiki" <torproject-admin@xxxxxxxxxxxxxx>]

#2091: Support ControlSocket as well as ControlPort
-------------------------+--------------------------------------------------
 Reporter:  arma         |       Owner:     
     Type:  enhancement  |      Status:  new
 Priority:  major        |   Milestone:     
Component:  Vidalia      |     Version:     
 Keywords:               |      Parent:     
-------------------------+--------------------------------------------------

Comment(by edmanm):

 Here's my comments on the patch, broken down by severity.

   1. [MAJOR] You've created a circular dependency between src/vidalia and
 src/torcontrol. src/torcontrol should NOT depend on anything in
 src/vidalia. You don't actually need to instantiate a TorSettings object
 in the ControlConnection class. You could simply pass the appropriate
 arguments to the connect() method based on whichever control method is
 configured.

   2. [MINOR] "Control" is a little too vague of a class name. Perhaps
 something like AbstractControlMethod, AbstractControlConnection,
 AbstractControlBase, or similar would be more descriptive of what that
 class actually does.

   3. [TRIVIAL] More 'stdset=0' stuff lurks in the .ui files.

   4. [MAJOR] The code common to both ControlSocket.cpp and ControlPort.cpp
 needs to be refactored out into your base class. It makes no sense to have
 huge chunks of identical code in both places, particularly since they both
 inherit from the same abstract base class.

   5. [MINOR] The "ControlMethod" setting should be a string, rather than
 just an enum number. If someone is looking at vidalia.conf,
 "ControlMethod=ControlSocket" is much clearer than "ControlMethod=1".  See
 how the authentication method settings are handled as a reference.
 TorSettings::getControlMethod() should still return the appropriate enum
 value, but the conversion happens "behind the scenes" in the TorSettings
 class.

   6.  [TRIVIAL] It looks like your added accessors getControlMethod() and
 getSocketPath() should be marked const.

   7. [TRIVIAL] The ControlMethod enum value passed as an argument to
 TorSettings::setControlMethod() doesn't actually need to be passed by
 const reference.

   8. [TRIVIAL] We try to prepend "_" to private member variables of a
 class, so they're easy to identify in their respective .cpp files. So,
 your "sock" variable in the ControlSocket class should be "_sock" (or even
 "_socket").

   9. [MINOR] It doesn't look like you're setting a default value for
 SETTING_SOCKET_PATH in the TorSettings constructor, even if it's only an
 empty string.


 I'd like to review your revised patch again before you commit it.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/2091#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs