mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
Date Thu, 06 Aug 2015 05:48:43 GMT

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 44)
<https://reviews.apache.org/r/36978/#comment148934>

    s/cmd/command/



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 45)
<https://reviews.apache.org/r/36978/#comment148935>

    s/cmd/command/
    s/empyt/empty/



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (lines 55 - 57)
<https://reviews.apache.org/r/36978/#comment148932>

    A couple of comments:
    
    (1) This is a good place for a 'std::initializer_list' to clean up the call sites, i.e.,
    
    os::shell("echo", {"hello", "world"});
    
    versus:
    
    os::shell("echo", vector<string>{"hello", "world"});
    
    (2) I'm a little confounded by the 'std::vector' approach (even after replacing with 'std::initializer_list').
In particular, it seems like the value it adds is that it keeps a programmer from having to
do 'strings::join(" ", args)' but they still have to stringify their arguments which makes
me wonder why you wouldn't just use 'strings::format' outside of the call everywhere? Or why
not keep the original "printf" version and call 'strings::format' internally instead of 'strings::join'?
With the 'std::initializer_list' approach I'll imagine we'll see a mix of:
    
    (A) os::shell("echo", {"hello", stringify(arg)});
    
    or:
    
    (B) os::shell("echo hello " + stringify(arg));
    
    or:
    
    (C) os::shell(strings::join(" ", "echo", "hello", stringify(arg)));
    
    or:
    
    (D) os::shell(strings::format("echo hello %s", arg));
    
    The existing version (or an improved variadic template version) would have looked something
like:
    
    (E) os::shell("echo hello %s", arg);
    
    I acknowledge that the 'std::initializer_list' version let's you add a third 'ignoreErrors'
parameter (which you couldn't do if instead of 'args' you used variadic parameters). But,
do we really need the 'ignoreErrors' parameter? Why not just ask people to append '|| exit
0' to their command just like we ask people to append '2>&1'? I like the idea of just
giving people a conduit to the shell, and however they'd do stuff in the shell world they
do here too.
    
    Now, if we were _not_ using 'popen' under the covers or passing a string to '/bin/sh'
then I would definitely see the value in a 'std::initializer_list' because then I wouldn't
need to quote the command! But unfortunately we're going to have to force people to quote
the command no matter what.
    
    Thus, my suggestion is to ask people to append '|| exit 0' and then go with a variadic
template implementation, i.e., (E), or if you want to be dead simple do (D) and then use 'strings::format'
everywhere in the next review.
    
    (Note that we use '%s' with our 'strings::format' for all types kind of like we use '{}'
in Python string templates.)



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp (line 31)
<https://reviews.apache.org/r/36978/#comment148920>

    s/cmd/command/ (matches the declaration).


- Benjamin Hindman


On Aug. 5, 2015, 7:56 a.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36978/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2015, 7:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3142
>     https://issues.apache.org/jira/browse/MESOS-3142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactoring os::shell.
> See MESOS-3142 for more details.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 4b7a7bafe1c64183d021b39cf12523250491f0ee

>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 2556bd428cc8990659e30e804b9c96c1659ef4a1

> 
> Diff: https://reviews.apache.org/r/36978/diff/
> 
> 
> Testing
> -------
> 
> make check
> *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: see also
https://reviews.apache.org/r/36979/
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>


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