[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #2046 [Tor Client]: Port Tor code for starting a background process to Windows



#2046: Port Tor code for starting a background process to Windows
------------------------+---------------------------------------------------
 Reporter:  sjmurdoch   |          Owner:  sjmurdoch         
     Type:  task        |         Status:  needs_review      
 Priority:  major       |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Client  |        Version:                    
 Keywords:              |         Parent:  #1983             
   Points:              |   Actualpoints:                    
------------------------+---------------------------------------------------

Comment(by nickm):

 From IRC:

 {{{

 20:19 < nickm> sjmurdoch: Initial request, pending more detailed review:
 Can we
                split the C string processing stuff out of functions like
                log_from_handle, so that we can do a unit test for it?
 20:21 < nickm> sjmurdoch: Also, elsewhere thoughout the code, we return
 structs
                on the heap, not by value. Any reason to make an exception
 for
                tor_spawn_background?
 20:21 < sjmurdoch> nickm: Yes, that sounds like a good idea
 20:21 < nickm> (If there's other string processing goop in the spawn code,
 same
                logic applies)
 20:21 < sjmurdoch> nickm: Not particularly
 20:22 < sjmurdoch> nickm: You mean pass in a pointer which the function
 will
                    modify?
 20:22 < sjmurdoch> or the function allocates the struct and returns a
 pointer?
 20:22 < nickm> Either one would be fine IMO.

 20:22 < sjmurdoch> nickm: OK
 20:24 < nickm> If the caller will generally just have it on the stack,
 passing
                a pointer is fine.
 20:24 < nickm> If the caller will generally want to have the struct stick
                around for a while, having the function allocate and return
 it
                is better.  You'd probably want to add a process_handle_t
                function somewhere as well.
 20:25 < nickm> sjmurdoch: (Also, is it okay if I copy-and-paste all of
 this
                onto the 2046 ticket when we're done?)
 20:25 < nickm> err, a process_handle_free function
 20:26 < sjmurdoch> Currently the process_handle_t instance for the helper
 is a
                    static variable in tor_check_port_forwarding
 20:26 < sjmurdoch> So I could pass in a pointer from
 tor_check_port_forwarding
                    to tor_spawn_background?
 20:27 < nickm> fine by me
 20:27 < sjmurdoch> nickm: And yes, feel free to paste into 2046
 20:28 < nickm> ok
 20:29 < nickm> so, what happens if some argv entry contains a space?
 20:29 < sjmurdoch> You're looking at the join code?
 20:30 < nickm> (Yeah.) That'll happen for sure if we ever need to run a
 program
                that lives under C:\Program Files\ .
 20:30 < sjmurdoch> For Windows, it would have to be put in quotes
 20:30 < nickm> sjmurdoch: I think we should have a separate, unit-tested
                function, that formats a command line properly for windows.
 20:31 < sjmurdoch> OK, I can refactor that out
 20:31 < nickm> great
 20:32 < nickm> also it should handle internal quotes; there have been way
 too
                many shell-based snafus in the history of programming for
 us to
                be blase about anything other than stricly correct command
 lines.
 20:32 < sjmurdoch> I don't fully understand how command lines are parsed
 on
                    Windows. I do know that it is the process which is
 called
                    that does it, but is it their libc?

 20:35 < nickm> I'm afraid I don't fully know.  But look at Python's
                subprocess.list2cmdline and at
                http://msdn.microsoft.com/en-us/library/ms880421
 20:35 < nickm> I think it's actually done in the equivalent of crt.o
 20:36 < sjmurdoch> Thanks, I'll take a look
 20:41 < nickm> For tor_get_exit_code: Let's return a define or an enum
 rather
                than a magic tristate.
 20:42 < sjmurdoch> Sounds sensible
 20:42 < nickm> nitpick: Should "log_from_handle" be called "log_to_handle"
 ?
 20:43 < sjmurdoch> I think log_from_handle because it reads from a handle
 and
                    logs the output
 20:43 < nickm> Ah!
 20:44 < nickm> makes sense
 20:44 < nickm> In tor_read_all_from_handle, I'd be more comfortable if it
 did
                an tor_assert(byte_count + numread <= count);

 20:47 < nickm> sjmurdoch: In tor_log_from_handle: Do we care about strings
 with
                embedded NULs?
 20:48 < sjmurdoch> nickm: Hmm, possibly
 20:48 < sjmurdoch> nickm: tor-fw-helper won't generate them but other
 processes
                    might
 20:49 < nickm> probably something to note; not necessarily something to
 fix
                right now.
 20:49 < sjmurdoch> OK
 20:49 < sjmurdoch> Related to this, I've assumed I'm working with byte
 strings
                    everywhere. Is Windows Unicode support going to bite
 me?
 20:50 < sjmurdoch> (another bit I don't understand well).
 20:50 < nickm> Not sure.  I think we're probably okay here.
 20:51 < nickm> Hm.  log_from_handle will do badly if the subprocess has
 mroe
                than 255 bytes to say.
 20:52 < sjmurdoch> I think it will output partial lines in that case
 20:52 < nickm> yeah.
 20:52 < nickm> It might do bogus newlines if the \r\n is split
 20:53 < sjmurdoch> Hmm, yes
 20:54 < sjmurdoch> I'm not sure what to do here. Code to handle partial
 lines
                    will be tricky. On *nix I got away with using fgets but
                    fgets on Windows can't ready from a handle
 20:55 < nickm> I'd say for now, document it as a known limitation
 20:55 < nickm> For later, you'd need to leave any partial line in a buffer
                associated with the handle
 20:55 < nickm> and make the whole thing more stateful
 20:56 < sjmurdoch> OK
 20:57 < sjmurdoch> nickm: There is already a ticket related to this:
                    https://trac.torproject.org/projects/tor/ticket/2045
 20:57 < nickm> sjmurdoch: .... and with that, I'm done with the initial
 review,
                I think.
 }}}

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