Re: [tor-bugs] #6411 [Tor]: Adding hidden services through control socket

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#6411: Adding hidden services through control socket
-------------------------+-------------------------------------------------
     Reporter:           |      Owner:  yawning
  kevinevans             |     Status:  needs_information
         Type:           |  Milestone:  Tor: 0.2.7.x-final
  enhancement            |    Version:  Tor: 0.2.3.19-rc
     Priority:  normal   |   Keywords:  hidden-service control maybe-
    Component:  Tor      |  proposal tor-hs globalleaks-wants
   Resolution:           |  Parent ID:  #8993
Actual Points:           |
       Points:           |
-------------------------+-------------------------------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_information


Comment:

 Review round 2! :)

 * in `rend_service_del_ephemeral()`, nitpick, consistent with code, this
 should be on the same line. Same in `handle_control_add_onion()`.
 {{{
 +  }
 +  SMARTLIST_FOREACH_END(circ);
 }}}

 * comment on top of `rend_service_add_ephemeral()` should mention what it
 does with `service_id_out`. Simply that on success, this is set with the
 newly created service_id value and it's the caller responsability to free
 it and on error, that value is still set to NULL also thus modified.

 * Being *very* picky, I would say that `DiscardPK` and `Detach` should
 also be in a static const.

 * Following the above, the good advantage of having them in a single
 declaration is that you can also use them in the error/log message. Like
 these (might have missed some):

  `connection_printf_to_buf(conn, "512 Invalid 'Flags' argument\r\n");`
  use "flags_prefix" instead of 'Flags'

  `connection_printf_to_buf(conn, "512 Missing 'Port' argument\r\n");`
  use "port_prefix" instead of 'Port'

  `connection_printf_to_buf(conn, "551 Failed to generate RSA key\r\n");`
  use "key_type_rsa1024", this one you are going to love it when ecc keys
 appears and you can just use the right static string without changing all
 error message hardcoded strings :).

 Ok I'm quite happy with this! Works well! If you have some cycles, some
 test(s) would be quite awesome ;). Once in Stem, we'll probably have
 regression test but unit test are always useful.

 Let me know about the next step but I would say this is ready for upstream
 (or at least for nickm to look at it). If you prefer a third review after
 some tweaks, feel free to ask! Good job!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6411#comment:52>
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