[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