mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 55241: Stop using os::system to validate perf event names.
Date Wed, 11 Jan 2017 09:53:04 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message