Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

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

(Updated May 12, 2016, 12:19 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


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


Repository: mesos


Description
---

Libprocess: Implemented `subprocess_windows.cpp`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
  3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
  3rdparty/libprocess/src/subprocess.cpp 
bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
  3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46608/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

> On May 11, 2016, 9:09 a.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/windows/subprocess.hpp, lines 122-181
> > 
> >
> > Follow-up to https://reviews.apache.org/r/46608/#comment195141: Since 
> > you say that we don't need to escape quotes, let's change `const 
> > vector& argv` to `vector argv` and all of this can be 
> > replaced with:
> > 
> > ```
> > string program = "\"" + argv[0] + "\"";
> > argv.erase(argv.begin());
> > string args = strings::join(" ", argv);
> > string cmd = strings::join(" ", program, args);
> > 
> > BOOL createProcessResult = CreateProcess(
> >   NULL,
> >   cmd.data(),
> >   ...);
> > ```

Talked with Michael and dpravat a bit offline. For the time being, we are going 
to expect that `argv` and `path` are correctly quoted when they are passed to 
the `subprocess` call. It might not be a permanent solution, but the naive 
solution of wrapping `argv[0]` in quotes is not going to cover all the corner 
cases. (For example, the path `C:\"Program Files"\foo.exe`.)

Our plan is to (1) just `strings::join(" ", argv)` and pass that as the command 
to `::CreateProcess`, (2) leave a `NOTE` on this code explaining that if you 
want to pass a binary with a space in it to `subprocess`, you get to quote that 
yourself for the time being, and (3) revisit this when we touch the executor, 
which will give us the opportunity to figure out how `subprocess` is being 
called, which will then help us decide how to handle this correctly, with 
minimum complexity -- our goal here is to _not_ reimplement the CLI parser.


- Alex


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


On May 10, 2016, 11:44 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 10, 2016, 11:44 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 122 - 181)


Follow-up to https://reviews.apache.org/r/46608/#comment195141: Since you 
say that we don't need to escape quotes, let's change `const vector& 
argv` to `vector argv` and all of this can be replaced with:

```
string program = "\"" + argv[0] + "\"";
argv.erase(argv.begin());
string args = strings::join(" ", argv);
string cmd = strings::join(" ", program, args);

BOOL createProcessResult = CreateProcess(
  NULL,
  cmd.data(),
  ...);
```


- Michael Park


On May 10, 2016, 11:44 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 10, 2016, 11:44 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

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

(Updated May 10, 2016, 11:44 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


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


Repository: mesos


Description
---

Libprocess: Implemented `subprocess_windows.cpp`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
  3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
  3rdparty/libprocess/src/subprocess.cpp 
bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
  3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46608/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

> On April 30, 2016, 1:04 a.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess_windows.cpp, lines 305-352
> > 
> >
> > (1) According to the "Security Remarks" section of 
> > https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx,
> >  it's recommended that we set the `lpApplicationName` parameter, rather 
> > than embedding the module name inside the `lpCommandLine` parameter since 
> > we need to make sure that the modulen name in `lpCommandLine` needs to be 
> > wrapped in quotes in case of spaces in the name. We don't seem to be doing 
> > this correctly.
> > 
> > (2) `argumentsEscaped` is `new`ed, but not `delete`d, and according to 
> > the documentation for `CreateProcess`, it does not call `delete` on 
> > `lpCommandLine`. It looks like we're leaking?
> > 
> > (3) Are we required to escape the quotes like we're doing here for 
> > `argumentsEscaped`...? I don't see anything like this in the documentation.
> > 
> > What I would prefer to do here I think is to take a `argv` by value, 
> > then do:
> > 
> > ```cpp
> > string program = argv[0];
> > argv.erase(argv.begin());
> > string args = strings::join(" ", argv);
> > 
> > ... = CreateProcess(
> >   program.data(),
> >   args.data(),
> >   ...);
> > ```
> > 
> > If we wanted to avoid using `lpApplicationName` for whatever reason, 
> > how about:
> > 
> > ```cpp
> > string program = "\"" + argv[0] + "\"";
> > argv.erase(argv.begin());
> > string args = strings::join(" ", argv);
> > string cmd = strings::join(" ", program, args);
> > 
> > ... = CreateProcess(
> >   NULL,
> >   cmd.data(),
> >   ...);
> > ```

To follow up, (2) is resolved. (3) will have semantics that are identical to 
`cmd.exe` for most cases we care about, so I think we do not need to escape 
quotes.

For (1) I think this is best addressed post MVP. As I mention in the `TODO` 
mapping arguments to `subprocess` into a `CreateProcess` call is nontrivial, 
and will need to be thought about carefully. We should come back to it post-MVP.


- Alex


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


On May 10, 2016, 8:10 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 10, 2016, 8:10 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

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

(Updated May 10, 2016, 8:10 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


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


Repository: mesos


Description
---

Libprocess: Implemented `subprocess_windows.cpp`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
  3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
  3rdparty/libprocess/src/subprocess.cpp 
bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
  3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46608/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

> On May 5, 2016, 9:29 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/windows/subprocess.hpp, line 59
> > 
> >
> > In `posix/subprocess.hpp`, we check for `>= 0` because we have `int` 
> > and `getOrElse(-1)`. Here we have `HANDLE` and 
> > `getOrElse(INVALID_HANDLE_VALUE)`. Is `>= 0` still the appropriate check?
> 
> Alex Clemmer wrote:
> Good catch.

Though it's a no-op if we close an invalid handle, but technically we should be 
checking because that's undocumented.


- Alex


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


On May 5, 2016, 3:12 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 5, 2016, 3:12 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

> On May 5, 2016, 9:29 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/windows/subprocess.hpp, line 59
> > 
> >
> > In `posix/subprocess.hpp`, we check for `>= 0` because we have `int` 
> > and `getOrElse(-1)`. Here we have `HANDLE` and 
> > `getOrElse(INVALID_HANDLE_VALUE)`. Is `>= 0` still the appropriate check?

Good catch.


- Alex


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


On May 5, 2016, 3:12 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 5, 2016, 3:12 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

> On May 5, 2016, 9:29 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/io.cpp, lines 291-295
> > 
> >
> > So for some reason `lambda::bind(::close, fd)` doesn't work? Do we 
> > know anything beyond "MSVC's `std::bind` is crippled"?
> > 
> > I think we could just do:
> > 
> > ```cpp
> > promise->future().onAny([fd]() { os::close(fd); });
> > ```
> > 
> > Here and below.

That's fine too. I don't know whether it's because `std::bind` is crippled, I 
don't really have an intuition for how this works. It is true that we have 
multiple `os::close` implementations now, though.


- Alex


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


On May 5, 2016, 3:12 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 5, 2016, 3:12 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Michael Park]

> On May 2, 2016, 9:14 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess_windows.cpp, line 414
> > 
> >
> > Remove newline.
> 
> Alex Clemmer wrote:
> I wasn't sure of intent here. You mean, remove all similar newlines? 
> because we do this in other lambdas, too.

The review's changed sufficiently that I don't remember which line this was.


- Michael


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


On May 5, 2016, 3:12 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 5, 2016, 3:12 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Michael Park]

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




3rdparty/libprocess/include/process/windows/subprocess.hpp (line 59)


In `posix/subprocess.hpp`, we check for `>= 0` because we have `int` and 
`getOrElse(-1)`. Here we have `HANDLE` and `getOrElse(INVALID_HANDLE_VALUE)`. 
Is `>= 0` still the appropriate check?



3rdparty/libprocess/src/io.cpp (lines 291 - 294)


So for some reason `lambda::bind(::close, fd)` doesn't work? Do we know 
anything beyond "MSVC's `std::bind` is crippled"?

I think we could just do:

```cpp
promise->future().onAny([fd]() { os::close(fd); });
```

Here and below.


- Michael Park


On May 5, 2016, 3:12 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 5, 2016, 3:12 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

> On May 2, 2016, 9:14 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess_windows.cpp, line 89
> > 
> >
> > The initialization with `INVALID_HANDLE_VALUE` has no semantic meaning, 
> > right? We should just leave it uninitialized, since we initialize it with 
> > the `duplicateHandle` call.

I think it makes sense to initialize it to a "safe" value, but it's fine to do 
it this way too, so let's do that.


> On May 2, 2016, 9:14 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess_windows.cpp, lines 199-200
> > 
> >
> > Is this because using `GENERIC_WRITE` with `FILE_SHARE_READ` is a no-op?

It's not a no-op, or I might not understand what you're saying. 
`FILE_SHARE_READ` will allow other processes to read the file, which in this 
case I think is what we want. I can add a comment.


> On May 2, 2016, 9:14 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess_windows.cpp, line 414
> > 
> >
> > Remove newline.

I wasn't sure of intent here. You mean, remove all similar newlines? because we 
do this in other lambdas, too.


- Alex


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


On May 5, 2016, 3:12 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 5, 2016, 3:12 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

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

(Updated May 5, 2016, 3:12 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


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


Repository: mesos


Description
---

Libprocess: Implemented `subprocess_windows.cpp`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
  3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
  3rdparty/libprocess/src/subprocess.cpp 
bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
  3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46608/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

> On April 28, 2016, 11:52 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess_windows.cpp, lines 534-539
> > 
> >
> > A few comments here.
> > 
> > (1) I think we can simply this to:
> > 
> > ```
> > string env;
> > foreach (const string& key, const string& value, environment.get()) {
> >   env += key + '=' + value + '\0';
> > }
> > ```
> > 
> > then pass `env.data()` to `createChildProcess`.
> > 
> > (2) Currently, if the string is larger than 32767 bytes, 
> > `createProcessEnvironment` returns `NULL`. We subsequently pass that result 
> > to `createChildProcess`, which result in the following behavior:
> > 
> > > lpEnvironment [in, optional]
> > A pointer to the environment block for the new process. If this 
> > parameter is NULL, the new process uses the environment of the calling 
> > process.
> > 
> > Is this what we want? I think we can definitely incorporate whatever 
> > behavior we need if we go beyond the 32767 bytes, but I'm not sure exactly 
> > what behavior is expected here.

We can do that. It was a bad failure mode to silently map environments that 
were too big to `NULL`. We should let `::CreateProcess` error out in that case 
which will cause us to return an `Error`.


- Alex


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


On May 3, 2016, 5:45 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 3, 2016, 5:45 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Daniel Pravat]

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




3rdparty/libprocess/src/subprocess.cpp (line 235)


You should close the thread handle in the `createChildProcess`



3rdparty/libprocess/src/subprocess.cpp (line 235)


You should close the thread handle in the `createChildProcess`


- Daniel Pravat


On May 3, 2016, 5:45 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated May 3, 2016, 5:45 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

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

(Updated May 3, 2016, 5:45 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


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


Repository: mesos


Description
---

Libprocess: Implemented `subprocess_windows.cpp`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
  3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
  3rdparty/libprocess/src/subprocess.cpp 
bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
  3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46608/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Michael Park]

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




3rdparty/libprocess/src/subprocess_windows.cpp (line 89)


The initialization with `INVALID_HANDLE_VALUE` has no semantic meaning, 
right? We should just leave it uninitialized, since we initialize it with the 
`duplicateHandle` call.



3rdparty/libprocess/src/subprocess_windows.cpp (lines 102 - 106)


No need for `else`:

```cpp
  if (!result) {
return WindowsError("Failed to duplicate handle of stdin file");
  }
  
  return duplicate;
```



3rdparty/libprocess/src/subprocess_windows.cpp (lines 128 - 132)


No need for `else`:

```cpp
  if (handle == INVALID_HANDLE_VALUE) {
return WindowsError("Failed to get `HANDLE` for file descriptor");
  }
  
  return handle;
```



3rdparty/libprocess/src/subprocess_windows.cpp (line 190)


`read/only` -> `read-only`



3rdparty/libprocess/src/subprocess_windows.cpp (lines 199 - 200)


Is this because using `GENERIC_WRITE` with `FILE_SHARE_READ` is a no-op?



3rdparty/libprocess/src/subprocess_windows.cpp (lines 300 - 302)


We seem to list `in`, `out`, `err` in that order. Should do the same here.



3rdparty/libprocess/src/subprocess_windows.cpp (line 414)


Remove newline.



3rdparty/libprocess/src/subprocess_windows.cpp (line 568)


`s/thecode/the code/`



3rdparty/libprocess/src/subprocess_windows.cpp (line 583)


`s/stdoutfds.read/stderrfds.read/`



3rdparty/libprocess/src/subprocess_windows.cpp (line 590)


Why not use a `unique_ptr` or `Owned` here?


- Michael Park


On April 23, 2016, 11:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated April 23, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Michael Park]

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




3rdparty/libprocess/src/subprocess_windows.cpp (lines 305 - 352)


(1) According to the "Security Remarks" section of 
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx,
 it's recommended that we set the `lpApplicationName` parameter, rather than 
embedding the module name inside the `lpCommandLine` parameter since we need to 
make sure that the modulen name in `lpCommandLine` needs to be wrapped in 
quotes in case of spaces in the name. We don't seem to be doing this correctly.

(2) `argumentsEscaped` is `new`ed, but not `delete`d, and according to the 
documentation for `CreateProcess`, it does not call `delete` on 
`lpCommandLine`. It looks like we're leaking?

(3) Are we required to escape the quotes like we're doing here for 
`argumentsEscaped`...? I don't see anything like this in the documentation.

What I would prefer to do here I think is to take a `argv` by value, then 
do:

```cpp
string program = argv[0];
argv.erase(argv.begin());
string args = strings::join(" ", argv);

... = CreateProcess(
  program.data(),
  args.data(),
  ...);
```

If we wanted to avoid using `lpApplicationName` for whatever reason, how 
about:

```cpp
string program = "\"" + argv[0] + "\"";
argv.erase(argv.begin());
string args = strings::join(" ", argv);
string cmd = strings::join(" ", program, args);

... = CreateProcess(
  NULL,
  cmd.data(),
  ...);
```


- Michael Park


On April 23, 2016, 11:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated April 23, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Michael Park]

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




3rdparty/libprocess/src/subprocess_windows.cpp (lines 534 - 539)


A few comments here.

(1) I think we can simply this to:

```
string env;
foreach (const string& key, const string& value, environment.get()) {
  env += key + '=' + value + '\0';
}
```

then pass `env.data()` to `createChildProcess`.

(2) Currently, if the string is larger than 32767 bytes, 
`createProcessEnvironment` returns `NULL`. We subsequently pass that result to 
`createChildProcess`, which result in the following behavior:

> lpEnvironment [in, optional]
A pointer to the environment block for the new process. If this parameter 
is NULL, the new process uses the environment of the calling process.

Is this what we want? I think we can definitely incorporate whatever 
behavior we need if we go beyond the 32767 bytes, but I'm not sure exactly what 
behavior is expected here.


- Michael Park


On April 23, 2016, 11:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated April 23, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Daniel Pravat]

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




3rdparty/libprocess/src/subprocess_windows.cpp (line 348)


This is not requered in the current codebase.



3rdparty/libprocess/src/subprocess_windows.cpp (line 606)


Since the process is not in suspended there is no need to resume here.



3rdparty/libprocess/src/subprocess_windows.cpp (line 607)


The handle for the thread can be closed in the createChildProcess()


- Daniel Pravat


On April 23, 2016, 11:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> ---
> 
> (Updated April 23, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.

[Alex Clemmer]

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

(Updated April 23, 2016, 11:41 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


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


Repository: mesos


Description
---

Libprocess: Implemented `subprocess_windows.cpp`.


Diffs
-

  3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46608/diff/


Testing
---


Thanks,

Alex Clemmer