> On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote: > > src/launcher/executor.cpp, line 148 > > <https://reviews.apache.org/r/15542/diff/9/?file=412608#file412608line148> > > > > Where is commandMessage used? > > > > Consider: > > string commandMessage = "command: " + strings::join(" ", args) + "\n";
The variable `commandMessage` is only used to set `commandPrintable`. > On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote: > > src/launcher/executor.cpp, line 196 > > <https://reviews.apache.org/r/15542/diff/9/?file=412608#file412608line196> > > > > Since commandVector does not store a vector, what about calling this > > _argv (since it's really just storing argv in another form for execvp to > > stomach). > > > > Why does this call to execvp need an intermediate variable, whereas the > > one in launcher does not? It needs an intermediate value because it seems unwise to assume a dereference like this is async signal safe. Launcher doesn't fork (as far as I can tell) so it's not an issue. > On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote: > > src/slave/process_isolator.cpp, line 272 > > <https://reviews.apache.org/r/15542/diff/9/?file=412611#file412611line272> > > > > Ditto, why does this one need the intermediate commandVector, but the > > one in launcher does not? It needs an intermediate value because it seems unwise to assume a dereference like this is async signal safe. Launcher doesn't fork (as far as I can tell) so it's not an issue. > On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote: > > src/launcher/launcher.cpp, line 212 > > <https://reviews.apache.org/r/15542/diff/9/?file=412609#file412609line212> > > > > Does this need a newline? > > > > Consider: > > > > string message = "Could not execute: " + strings::join(" ", args) + > > "\n"; Other calls to `fatalerror` don't append newlines; `fatalerror` seems to add them (`fprintf(stderr, " (%s:%u)\n", ...)`). - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15542/#review31536 ----------------------------------------------------------- On Jan. 15, 2014, 11:06 p.m., Jason Dusek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15542/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2014, 11:06 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > Offer an execvp like interface for running tasks. > > Review: https://reviews.apache.org/r/15542 > > > Diffs > ----- > > include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34 > src/examples/python/test_framework.py > deca48e779ae099424fa73bb9a8ac5c419c5faf1 > src/launcher/executor.cpp b73ab479500a7347a38ba53acecfab9229f1080d > src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af > src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf > src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01 > > Diff: https://reviews.apache.org/r/15542/diff/ > > > Testing > ------- > > Ran Python test executor and `make check`. > > > Thanks, > > Jason Dusek > >
