This is an automatically generated e-mail. To reply, visit:
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 22
Policy is to include these by including `windows.hpp`. The reasoning is
that some of the win API headers are order-dependent and it makes it easier if
we know all the Windows-specific header files are in one place. If we start
including them directly, subtle things can break, like the `#undef` strategy we
use for stuff like `GetMessage`.
So, could we please use that strategy here also instead of directly
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 25
Can we please avoid defining global-scope macros in public headers? It
seems like we could do something more `const`-flavored here?
Also, it looks like this is just to give names for the return values of
`kill`? That's fine, I just want to make sure I understand.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 27)
No big deal but we typically leave 2 lines between "header stuff" and the
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 30)
If we don't mean to expose `kill_process` here, can we please put it in the
Beyond the fact that I want to keep the `os::` namespace clean, it's
important to note that out `pid_t` is unsigned, while most POSIX
implementations are signed. So if we expose a lot of APIs that take `pid_t` as
argument, people could expect that they take negative numbers (as, for example,
`kill` does). I realize this is a programming surface that exists only on
Windows, but one of the ways I want to limit risk is by really buckling down on
APIs that expose some form of `pid_t` because all of those uses could lead to
risky behavior, so it's best to have as few of them floating around as we can,
in general. So relative to the amount of work here, I think the payoff is high.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 34)
I'll make this comment once, but it applies to a couple places in the code
My recollection is that the \ character doesn't get rid of the leading
whitespace on the next line. Is that right? If so then this would appear in the
logs like this:
os::TerminateProcess(): Failed call to OpenProcess."
If that's correct, can we please transition to something like this (or, if
you prefer, some sort of C++ concatenation):
LOG(ERROR) << (WindowsError("os::TerminateProcess(): Failed call to "
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 39)
Could we please use `os::close` here instead of `::CloseHandle`?
(And, Joris is right, it is better to use `SharedHandle`, I think. :) )
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 41)
Nit: Could combine this with the line above instead of having an
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 44)
Small nit, by the way, but we're using an error format like
`os::internal::kill_process: error message here`.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 51)
Can we please add 2 newlines between functions? (General comment, this will
not be relevant when we move `kill_process` to its own namespace.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 52)
Because `pid_t` is signed on POSIX and unsigned on Windows, we need to very
purposeful about the behavior here. You can see our approach to `waitpid` in a
previous review: https://reviews.apache.org/r/46340/
Let me say two things in refernce to this issue.
First, in this case I think it is arguable that we are being overly
cautious with the `pid_t` handling here, because some CRT APIs do use `int` to
represent `pid`s (for example `_getpid` does this). In this case, extra caution
costs is basically nothing (just the cost of writing a couple comments) and the
result that we get out of it is (1) no subtle bugs, and (2) a junior engineer
can maintain the code and understand fully the assumptions that would otherwise
be implicit in the code. I personally would recommend a similar approach to
this in `kill`.
Second, my belief is that the "weird" numbers like `-1` are not used
pervasively in the Mesos codebase, so I see nothing particularly wrong with
limiting the set of arguments we are using in the same way we did with
Curious to hear your thoughts.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 65
Huh, does that work? I thought `errno` was a macro on Windows, pointing at
a function. It seems like we should be using `_set_errno` instead?
3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 319)
Is this a part of this review? I don't see it used anywhere?
3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 321 - 323)
Seems like the comment wrapping here is not at 80 columns?
3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 363 - 367)
Also looks like it's not a part of the code review? Dont' see it used
3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 396 - 397)
I think these are semantically identical, so we might even just add theese
as constants above, in the same way we do for other stuff like `S_IRUSR`.
- Alex Clemmer
On May 10, 2016, 8:54 a.m., Daniel Pravat wrote:
> This is an automatically generated e-mail. To reply, visit:
> (Updated May 10, 2016, 8:54 a.m.)
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van
> Remoortere, and Michael Park.
> Repository: mesos
> Windows: Implement `kill` and `killtree`.
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/kill.hpp PRE-CREATION
> Diff: https://reviews.apache.org/r/47168/diff/
> Windows: build/test w/ Marathon
> Daniel Pravat