mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 55239: Stop using os::system to extract fetcher archives.
Date Wed, 11 Jan 2017 16:12:07 GMT


> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > I don't feel that we need to introduce these wrappers, which are either single-use
or don't provide a better abstration than the counterpart in stout.
> > 
> > Can we uniformly use `subprocess` in all cases (since `os::spawn()` doesn't work
in all cases) for consistency?
> > 
> > To reuse code, we can do this
> > 
> > ```
> > vector<string> command;
> > 
> > // Potentially need to specify destination via stdout.
> > Option<Subprocess::IO> out = None();
> > 
> > // if tar.
> > command = {"tar", "-C", destinationDirectory, "-xf", sourcePath};
> > // else if gzip.
> > command = {"gzip", "-dc", sourcePath};
> > out = Subprocess::PATH(destinationPath);
> > // else if zip.
> > command = {"unzip", "-o", "-d", destinationDirectory, sourcePath};
> > // else
> > 
> > ...
> > 
> > // Comment that we use the argv of subprocess to avoid injection.
> > Try<Subprocess> s = subprocess(
> >     command[0],
> >     command,
> >     Subprocess::FD(STDIN_FILENO),
> >     out.getOrElse(Subprocess::FD(STDOUT_FILENO)));
> > ```
> > 
> > What do you think?

We could subprocess everything. Would need to think about how to format the command message
legibly.


> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 85
> > <https://reviews.apache.org/r/55239/diff/1/?file=1598134#file1598134line85>
> >
> >     This is the default so unnecessary?

However it makes it obvious to the reader, so it should be kept.


> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 95
> > <https://reviews.apache.org/r/55239/diff/1/?file=1598134#file1598134line95>
> >
> >     `stringify` on `vector` puts `", "` between items.

Yes, that's desirable so that you can see the whitespace in the command.


> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 67
> > <https://reviews.apache.org/r/55239/diff/1/?file=1598134#file1598134line67>
> >
> >     `stringify` on `vector` puts `", "` between items. We can use `strings::join()`.

This is intended since it makes the actual arguments much clearer and more obvious.


- James


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


On Jan. 6, 2017, 1:13 a.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2017, 1:13 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
>     https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Stop using os::system to extract fetcher archives.
> 
> 
> Diffs
> -----
> 
>   src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
> 
> Diff: https://reviews.apache.org/r/55239/diff/
> 
> 
> Testing
> -------
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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