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




3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 22 
- 23)
<https://reviews.apache.org/r/47168/#comment196682>

    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 
including them?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 25 
- 26)
<https://reviews.apache.org/r/47168/#comment196689>

    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)
<https://reviews.apache.org/r/47168/#comment196705>

    No big deal but we typically leave 2 lines between "header stuff" and the 
namespace.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 30)
<https://reviews.apache.org/r/47168/#comment196683>

    If we don't mean to expose `kill_process` here, can we please put it in the 
`internal::os::` namespace?
    
    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)
<https://reviews.apache.org/r/47168/#comment196688>

    I'll make this comment once, but it applies to a couple places in the code 
below.
    
    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 "
                                      "OpenProcess.")).message;
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 39)
<https://reviews.apache.org/r/47168/#comment196690>

    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)
<https://reviews.apache.org/r/47168/#comment196691>

    Nit: Could combine this with the line above instead of having an 
intermediate value.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 44)
<https://reviews.apache.org/r/47168/#comment196709>

    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)
<https://reviews.apache.org/r/47168/#comment196700>

    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)
<https://reviews.apache.org/r/47168/#comment196704>

    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 
`os::waitpid`.
    
    Curious to hear your thoughts.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 65 
- 66)
<https://reviews.apache.org/r/47168/#comment196708>

    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)
<https://reviews.apache.org/r/47168/#comment196711>

    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)
<https://reviews.apache.org/r/47168/#comment196712>

    Seems like the comment wrapping here is not at 80 columns?



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 363 - 367)
<https://reviews.apache.org/r/47168/#comment196713>

    Also looks like it's not a part of the code review? Dont' see it used 
anywhere?



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 396 - 397)
<https://reviews.apache.org/r/47168/#comment196714>

    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:
> https://reviews.apache.org/r/47168/
> -----------------------------------------------------------
> 
> (Updated May 10, 2016, 8:54 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Implement `kill` and `killtree`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 33ddb06e25920096f2d16d4f372129ee2f6a8721 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/kill.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/kill.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 3828c53c0dbbf370d642189998af75b9af434e9d 
> 
> Diff: https://reviews.apache.org/r/47168/diff/
> 
> 
> Testing
> -------
> 
> Windows: build/test w/ Marathon
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Reply via email to