mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Akash Gupta <akash-gu...@hotmail.com>
Subject Re: Review Request 63859: Windows: Fixed mock signal values in stout.
Date Thu, 11 Jan 2018 21:56:17 GMT


> On Jan. 10, 2018, 12:09 p.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/windows.hpp
> > Lines 343-348 (original), 343-348 (patched)
> > <https://reviews.apache.org/r/63859/diff/5/?file=1931096#file1931096line343>
> >
> >     Could you please clarify, what the fix is here? At least in the patch description,
but feel free to update the comment above.
> 
> Andrew Schwartzmeyer wrote:
>     The clarification is in the previous comments on the review: these were literally
incorrect values. They're now correct. I don't know how to link you to the comment, but I'll
reproduce it here:
>     
>     Me:
>     
>     > :/ I'd really like to fix whatever code is using these signals for logic. I
feel like the defining the signals for Windows was originally a band-aid, and understand this
patch didn't add them.
>     > The funny thing is that, since these values aren't defined on Windows, they
could be any number so long as only the symbol is used in the rest of the code base. I think
this is why this worked anyway.
>     > Akash, what bug did you run into that required correcting these?
>     > (And indeed, they now appear correct. SIGKILL is 9, SIGCONT is 18 in decimal,
and SIGSTOP is 19 in decimal).
>     
>     Akash:
>     
>     > I think they worked before because they were used only within stout. One of
the docker executor method signature is docker->kill(ID, SIGNAL), which eventually calls
docker kill -s SIGNAL ID. Docker (and go standard library) defines the Linux signal values
on Windows, so it's expecting docker kill -s 9. If you want, I can fix the docker executor
to ignore the signal field on Windows and just send docker kill without the -s.
>     
>     Me:
>     
>     > Gotcha! That makes sense. I could go either way on your proposition. It obviously
works and is supported by Docker on Windows, so it makes sense (now) to leave it. OTOH if
removing it would also let us remove these definitions, I like that too.
> 
> Andrew Schwartzmeyer wrote:
>     (I mean, the fix was to fix the literal values coded here to match what `SIGKILL`
etc. are defined to be, the original author goofed.)
> 
> Akash Gupta wrote:
>     Yeah, the code comment above the signal defines is `// Linux signal flags not used
in Windows. We define them per 'Linux sys/signal.h' to branch properly for Windows processes'
stop, resume and kill.` The original code wasn't defining the correct signal values  based
off Linux's `sys/signal.h`, so this change just changes it to the correct ones.
> 
> Alexander Rukletsov wrote:
>     Thanks guys! What I meant was `SIGCONT` and `SIGSTOP` do not have portable numbers.
For example, unix reference (and my mac) define	`SIGSTOP 17` and `SIGCONT 19`, while Linux,
e.g. Fedora 25 I have at hand, goes for `SIGCONT 18` and `SIGSTOP 19`. Hence I'd like to understand
why you pick the linux option and have it captured in a comment for posterity.

Andy and I discussed this and we found that `SIGCONT` and `SIGSTOP` aren't even used in the
Windows Mesos agent. We decided to simply remove them, since those signals aren't defined
on Windows. `SIGKILL` should be kept, because it's used a bunch and docker defines SIGKILL
= 9 on Windows.


- Akash


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


On Jan. 5, 2018, 12:28 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2018, 12:28 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
>     https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Also fixed the WEXITSTATUS macro to cast the exit code instead of
> bit-masking it, since Windows exit codes are 32 bit unsigned ints.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/windows.hpp 7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 
> 
> 
> Diff: https://reviews.apache.org/r/63859/diff/5/
> 
> 
> Testing
> -------
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>


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