Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 16, 2018, 8:54 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> ---
> 
> (Updated Jan. 16, 2018, 8:54 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
> ---
> 
> Removed `SIGSTOP` and `SIGCONT` on Windows, since they are meaningless,
> and never unused. 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/os/windows/kill.hpp 
> b2a36b5c07df6642bb9b08b61ab3b678bf8a6b36 
>   3rdparty/stout/include/stout/windows.hpp 
> 7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 
> 
> 
> Diff: https://reviews.apache.org/r/63859/diff/7/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

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

(Updated Jan. 16, 2018, 4:54 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

Fix typos


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


Repository: mesos


Description
---

Removed `SIGSTOP` and `SIGCONT` on Windows, since they are meaningless,
and never unused. 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 (updated)
-

  3rdparty/stout/include/stout/os/windows/kill.hpp 
b2a36b5c07df6642bb9b08b61ab3b678bf8a6b36 
  3rdparty/stout/include/stout/windows.hpp 
7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 


Diff: https://reviews.apache.org/r/63859/diff/7/

Changes: https://reviews.apache.org/r/63859/diff/6-7/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

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

(Updated Jan. 12, 2018, 2:33 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

Removed SIGSTOP and SIGCONT


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


Repository: mesos


Description (updated)
---

Removed `SIGSTOP` and `SIGCONT` on Windows, since they are meaningless,
and never unused. 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 (updated)
-

  3rdparty/stout/include/stout/os/windows/kill.hpp 
b2a36b5c07df6642bb9b08b61ab3b678bf8a6b36 
  3rdparty/stout/include/stout/windows.hpp 
7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 


Diff: https://reviews.apache.org/r/63859/diff/6/

Changes: https://reviews.apache.org/r/63859/diff/5-6/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

> On Jan. 10, 2018, 12:09 p.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/windows.hpp
> > Lines 343-348 (original), 343-348 (patched)
> > 
> >
> > 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
> 
>

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Alexander Rukletsov]

> On Jan. 10, 2018, 12:09 p.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/windows.hpp
> > Lines 343-348 (original), 343-348 (patched)
> > 
> >
> > 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.

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.


- Alexander


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

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

> On Jan. 10, 2018, 12:09 p.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/windows.hpp
> > Lines 343-348 (original), 343-348 (patched)
> > 
> >
> > 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.)

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.


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

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Andrew Schwartzmeyer]

> On Jan. 10, 2018, 4:09 a.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/windows.hpp
> > Lines 343-348 (original), 343-348 (patched)
> > 
> >
> > 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.

(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.)


- Andrew


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


On Jan. 4, 2018, 4:28 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> ---
> 
> (Updated Jan. 4, 2018, 4:28 p.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
> 
>

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Andrew Schwartzmeyer]

> On Jan. 10, 2018, 4:09 a.m., Alexander Rukletsov wrote:
> > 3rdparty/stout/include/stout/windows.hpp
> > Lines 343-348 (original), 343-348 (patched)
> > 
> >
> > Could you please clarify, what the fix is here? At least in the patch 
> > description, but feel free to update the comment above.

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


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


On Jan. 4, 2018, 4:28 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> ---
> 
> (Updated Jan. 4, 2018, 4:28 p.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
> 
>

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Alexander Rukletsov]

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




3rdparty/stout/include/stout/windows.hpp
Lines 343-348 (original), 343-348 (patched)


Could you please clarify, what the fix is here? At least in the patch 
description, but feel free to update the comment above.


- Alexander Rukletsov


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

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

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


Changes
---

Rebased.


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 (updated)
-

  3rdparty/stout/include/stout/windows.hpp 
7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 


Diff: https://reviews.apache.org/r/63859/diff/5/

Changes: https://reviews.apache.org/r/63859/diff/4-5/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Dec. 7, 2017, 4:02 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> ---
> 
> (Updated Dec. 7, 2017, 4:02 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/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

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

(Updated Dec. 7, 2017, 12:02 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

Rebase


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 (updated)
-

  3rdparty/stout/include/stout/windows.hpp 
7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 


Diff: https://reviews.apache.org/r/63859/diff/4/

Changes: https://reviews.apache.org/r/63859/diff/3-4/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Andrew Schwartzmeyer]

> On Nov. 30, 2017, 6:14 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/windows.hpp
> > Lines 343-348 (original), 343-348 (patched)
> > 
> >
> > :/ 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 Gupta wrote:
> 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`.

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


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


On Nov. 27, 2017, 9:36 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> ---
> 
> (Updated Nov. 27, 2017, 9:36 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/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

> On Dec. 1, 2017, 2:14 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/windows.hpp
> > Lines 343-348 (original), 343-348 (patched)
> > 
> >
> > :/ 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).

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


- Akash


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


On Nov. 27, 2017, 5:36 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> ---
> 
> (Updated Nov. 27, 2017, 5:36 p.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/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Andrew Schwartzmeyer]

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




3rdparty/stout/include/stout/windows.hpp
Lines 343-348 (original), 343-348 (patched)


:/ 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).



3rdparty/stout/include/stout/windows.hpp
Lines 376-377 (original), 376-378 (patched)


I wish I had context as to why this was a bitmask originally... I'll do a 
git blame and see what I can dig up.


- Andrew Schwartzmeyer


On Nov. 27, 2017, 9:36 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63859/
> ---
> 
> (Updated Nov. 27, 2017, 9:36 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/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

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

(Updated Nov. 27, 2017, 5:36 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

rebased


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 (updated)
-

  3rdparty/stout/include/stout/windows.hpp 
7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 


Diff: https://reviews.apache.org/r/63859/diff/3/

Changes: https://reviews.apache.org/r/63859/diff/2-3/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63859: Windows: Fixed mock signal values in stout.

[Akash Gupta]

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

(Updated Nov. 17, 2017, 10:35 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Summary (updated)
-

Windows: Fixed mock signal values in stout.


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


Repository: mesos


Description (updated)
---

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 (updated)
-

  3rdparty/stout/include/stout/windows.hpp 
7aa0ba72c4cc3b688ce6374b3308945ea8cb7572 


Diff: https://reviews.apache.org/r/63859/diff/2/

Changes: https://reviews.apache.org/r/63859/diff/1-2/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta