----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55241/#review161194 -----------------------------------------------------------
It's worth noting that the perf events are specified via agent flags so no unprivileged / remote user input is involved but I guess the goal is to eliminate all instances of `os::system()` (at least the ones with any non-static commands)? Are the reviews in this JIRA combined achieving that? src/linux/perf.cpp (line 320) <https://reviews.apache.org/r/55241/#comment232432> s/{ "perf", "stat", "--log-fd", "2" }/{"perf", "stat", "--log-fd", "2"}/ src/linux/perf.cpp (line 327) <https://reviews.apache.org/r/55241/#comment232433> What does `true` do in here? src/linux/perf.cpp (lines 329 - 341) <https://reviews.apache.org/r/55241/#comment232434> I am not sure how much I like the `internal::Perf` abtraction but it's there and other similar calls in this file use it so we might as well just do: ``` internal::Perf* perf = new internal::Perf(argv); Future<string> output = perf->output(); spawn(perf, true); output.await(); // The output being ready means the the command is successful and the events are valid. return output.isReady(); ``` src/linux/perf.cpp (line 333) <https://reviews.apache.org/r/55241/#comment232435> I guess we don't need to see stdout? - Jiang Yan Xu On Jan. 5, 2017, 5:17 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55241/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2017, 5:17 p.m.) > > > Review request for mesos, Benjamin Mahler, Greg Mann, and Jiang Yan Xu. > > > Bugs: MESOS-6862 > https://issues.apache.org/jira/browse/MESOS-6862 > > > Repository: mesos > > > Description > ------- > > Stop using os::system to validate perf event names. > > > Diffs > ----- > > src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f > > Diff: https://reviews.apache.org/r/55241/diff/ > > > Testing > ------- > > `sudo make check` (Fedora 25) > > > Thanks, > > James Peach > >
