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