Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 21, 2016, 4:07 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
a81086860a17bd1912e55051dd7bb98a20772b51 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
f1584d0786ee0236a0e703d4aa9532e138ad49f0 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
37ef33257830e32875500537df7af38757c6efac 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review129642
---


Ship it!




Ship It!

- Michael Park


On April 18, 2016, 7:09 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 18, 2016, 7:09 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 18, 2016, 7:09 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 17, 2016, 10:18 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

> On April 15, 2016, 11:31 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 362
> > 
> >
> > What's the difference between `Failed to call X` vs `Call to `X` 
> > failed`?

Ah, I tried to be consistent about making all these messages say `Call to x 
failed` because I thought it was clearer. That is why there are both -- I just 
didn't get all of them.

Also, this message is somewhat inconsistent with our style. We say 
`os::processes: Call to x failed`. So we should fix that here too.


> On April 15, 2016, 11:31 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, lines 
> > 406-407
> > 
> >
> > I think it would easier to read:
> > 
> > ```
> > Try utime = Nanoseconds(lKernelTime.QuadPart * 100);
> > Try stime = Nanoseconds(lUserTime.QuadPart * 100);
> > ```
> > 
> > This is also arguably more correct, since any number < 10,000,000 would 
> > incorrectly result in 0 by the division.
> > 
> > What do you think?

You're right, this was sloppy.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review129204
---


On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 15, 2016, 7:50 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review129204
---




3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 294)


Should we use `SharedHandle` here?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 312 - 
313)


`} else {`



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 359)


What's the difference between `Failed to call X` vs `Call to `X` failed`?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 403 - 
404)


I think it would easier to read:

```
Try utime = Nanoseconds(lKernelTime.QuadPart * 100);
Try stime = Nanoseconds(lUserTime.QuadPart * 100);
```

This is also arguably more correct, since any number < 10,000,000 would 
incorrectly result in 0 by the division.

What do you think?


- Michael Park


On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 15, 2016, 7:50 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Michael Park]

> On April 14, 2016, 9:10 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, lines 173-183
> > 
> >
> > (1) It seems like this should live in `posix/os.hpp`.
> > (2) The declaration of this function in `windows/os.hpp` is:
> > ```
> > inline Result process(pid_t pid);
> > ```
> > It seems like the windows version needs all 3 states whereas the POSIX 
> > version only needs 2?
> > If that is the case, I think it makes sense to consolidate to using 
> > `Result` in both cases and
> > leave a comment on the POSIX version that we have the 3 states because 
> > of Windows.
> 
> Alex Clemmer wrote:
> I think I must be missing something. There is a version of `process` that 
> returns a `Result` in `linux.hpp`, `osx.hpp`, and so on -- the version whose 
> prototype you mention here is just the Windows implementation of this.
> 
> For the function you are highlighting here in your comment, which returns 
> `Option`, this is separate -- it is meant to be shared among all platforms. 
> There is no need for it to return `Result` because it is just finding a 
> process with a certain `pid` in a list of processes.
> 
> So, my recommendation is to keep it the way it is.
> 
> Thoughts?

Yep, you're right. I read incorrectly. Too bad that we have `Option 
process(pid_t, const std::list&);` as well as `Result 
process(pid_t);` but that's a separate issue :)


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128983
---


On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 15, 2016, 7:50 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review129192
---


Ship it!




Ship It!

- Michael Park


On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 15, 2016, 7:50 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

> On April 14, 2016, 9:10 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, lines 173-183
> > 
> >
> > (1) It seems like this should live in `posix/os.hpp`.
> > (2) The declaration of this function in `windows/os.hpp` is:
> > ```
> > inline Result process(pid_t pid);
> > ```
> > It seems like the windows version needs all 3 states whereas the POSIX 
> > version only needs 2?
> > If that is the case, I think it makes sense to consolidate to using 
> > `Result` in both cases and
> > leave a comment on the POSIX version that we have the 3 states because 
> > of Windows.

I think I must be missing something. There is a version of `process` that 
returns a `Result` in `linux.hpp`, `osx.hpp`, and so on -- the version whose 
prototype you mention here is just the Windows implementation of this.

For the function you are highlighting here in your comment, which returns 
`Option`, this is separate -- it is meant to be shared among all platforms. 
There is no need for it to return `Result` because it is just finding a process 
with a certain `pid` in a list of processes.

So, my recommendation is to keep it the way it is.

Thoughts?


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128983
---


On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 15, 2016, 7:50 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 15, 2016, 7:50 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128983
---




3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 155)


Remove newline.



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 161)


`s/foreach(/foreach (/`



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (lines 173 - 183)


(1) It seems like this should live in `posix/os.hpp`.
(2) The declaration of this function in `windows/os.hpp` is:
```
inline Result process(pid_t pid);
```
It seems like the windows version needs all 3 states whereas the POSIX 
version only needs 2?
If that is the case, I think it makes sense to consolidate to using 
`Result` in both cases and
leave a comment on the POSIX version that we have the 3 states because of 
Windows.



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 200)


`s/foreach(/foreach (/`



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 202)


Indent 2 more spaces.



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 214)


`s/> >/>>/`



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 217)


Remove newline.


- Michael Park


On April 14, 2016, 8:57 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 14, 2016, 8:57 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 14, 2016, 8:57 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Naparu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128781
---




3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 310)


`break` is not needed after `return`


- Alex Naparu


On April 13, 2016, 10:06 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 13, 2016, 10:06 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 13, 2016, 10:06 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Naparu]

> On April 12, 2016, 11:10 p.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 383
> > 
> >
> > Can you document this flag a bit please? GetProcessTimes requires 
> > PROCESS_QUERY_LIMITED_INFORMATION, while GetProcessMemoryInfo requires the 
> > additional PROCESS_VM_READ.
> 
> Alex Clemmer wrote:
> Sure, but what part would you like me to document?

I would like to see some explanatory note as to why the THREAD_ALL_ACCESS flag 
is required (and appropriate) in this context. Looking at MSDN, none of the 
functions that are using the process handle (GetProcessTimes and 
GetProcessMemoryInfo) make any note of this flag. It might be that 
THREAD_ALL_ACCESS is overkill in this context.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128565
---


On April 13, 2016, 9:49 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 13, 2016, 9:49 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

> On April 12, 2016, 11:10 p.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 383
> > 
> >
> > Can you document this flag a bit please? GetProcessTimes requires 
> > PROCESS_QUERY_LIMITED_INFORMATION, while GetProcessMemoryInfo requires the 
> > additional PROCESS_VM_READ.

Sure, but what part would you like me to document?


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128565
---


On April 13, 2016, 9:33 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 13, 2016, 9:33 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 13, 2016, 9:49 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

> On April 12, 2016, 11:10 p.m., Alex Naparu wrote:
> >

Hey, sorry, you reviewed an old version of the code. I had a different branch 
checked out when I pushed this. The background is that this is an amalgamation 
of maybe 3 or 4 commits, and the code is not amazing. I went through and fixed 
up a bunch of stuff on behalf of others, since they don't have the cycles to 
deal with it themselves.

Anyway, all this is to say: I've pushed a new revision, so most of these 
comments will be addressed incidentally -- things like `exists` being a 
parameter, stout naming conventions, etc.

Sorry for the confusion.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128565
---


On April 13, 2016, 9:33 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 13, 2016, 9:33 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 13, 2016, 9:33 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Naparu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128565
---




3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 164)


I assume "disappear" means "between enumeration and now". Can you expand 
this comment a bit please?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 270)


Doesn't follow stout naming convention (and can be mistaken for a WinAPI 
function).



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 272)


Not entirely sure we need this output arg



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 286)


spacing



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 289)


Comparing a HANDLE to NULL is a risky thing to do. I don't think MSDN 
defines NULL (which is defined as 0 in WinAPI) as an invalid handle value. We 
might just trigger a false negative here. Any HANDLE result should only be 
tested against INVALID_HANDLE_VALUE.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 294)


I think we have some sort of safe_handle type by now, right?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 304)


This can probably be scoped to each code block. No need to define it here, 
just creates confusion.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 339)


These can all be on the same line.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 372)


I don't think we should be using this flag. The fact that FindProcess 
returned an error is a solid indication that something is wrong.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 382)


Can you qualify these calls with `::` please?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 383)


Can you document this flag a bit please? GetProcessTimes requires 
PROCESS_QUERY_LIMITED_INFORMATION, while GetProcessMemoryInfo requires the 
additional PROCESS_VM_READ.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 399)


Extra newline?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 419)


Newline



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 423)


I don't think these can be negative, consider using ULARGE_INTEGER instead.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 429)


Do you need system_time and user_time? I think these can be inlined below.


- Alex Naparu


On April 11, 2016, 7:19 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 11, 2016, 7:19 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 11, 2016, 7:19 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer