You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/02/25 23:31:44 UTC
Review Request 31443: Accept dummy arguments for fs::mount().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31443/
-----------------------------------------------------------
Review request for mesos, Dominic Hamon and Jie Yu.
Repository: mesos
Description
-------
Accept dummy arguments for fs::mount().
Diffs
-----
src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
Diff: https://reviews.apache.org/r/31443/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 31443: Accept dummy arguments for fs::mount().
Posted by Ian Downes <ia...@gmail.com>.
> On Feb. 25, 2015, 2:48 p.m., Dominic Hamon wrote:
> > src/linux/fs.cpp, line 162
> > <https://reviews.apache.org/r/31443/diff/1/?file=876476#file876476line162>
> >
> > maybe pull these out into variables for better readability.
> >
> > const char* source_str = source.isSome() ? source.get().c_str() : NULL;
> > const char* type_str = type.isSome() ? type.get().c_str() : NULL;
> >
> > if (::mount(source_str, target.c_str, type_str, flags, data) < 0) {
> > ...
> >
> >
> > also, does it make sense for type to be non-null if source is null? and vice versa?
Sure, you can leave the type as null if you're bind mounting (it'll inherit the source's type), and you can leave the source null if you're modifying an existing mount.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31443/#review74116
-----------------------------------------------------------
On Feb. 25, 2015, 2:31 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31443/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2015, 2:31 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Accept dummy arguments for fs::mount().
>
>
> Diffs
> -----
>
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
>
> Diff: https://reviews.apache.org/r/31443/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 31443: Accept dummy arguments for fs::mount().
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31443/#review74116
-----------------------------------------------------------
src/linux/fs.cpp
<https://reviews.apache.org/r/31443/#comment120637>
maybe pull these out into variables for better readability.
const char* source_str = source.isSome() ? source.get().c_str() : NULL;
const char* type_str = type.isSome() ? type.get().c_str() : NULL;
if (::mount(source_str, target.c_str, type_str, flags, data) < 0) {
...
also, does it make sense for type to be non-null if source is null? and vice versa?
- Dominic Hamon
On Feb. 25, 2015, 2:31 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31443/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2015, 2:31 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Accept dummy arguments for fs::mount().
>
>
> Diffs
> -----
>
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
>
> Diff: https://reviews.apache.org/r/31443/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 31443: Accept dummy arguments for fs::mount().
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31443/
-----------------------------------------------------------
(Updated March 10, 2015, 10:24 a.m.)
Review request for mesos, Dominic Hamon and Jie Yu.
Repository: mesos
Description
-------
Accept dummy arguments for fs::mount().
Diffs (updated)
-----
src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
src/slave/containerizer/isolators/namespaces/pid.cpp 7e72b34c7bf5f0b96a2f62db138972d44e084c8b
src/slave/containerizer/isolators/network/port_mapping.cpp 5227987cdb7b904c2f4bb2bdf5c5d705a435010d
Diff: https://reviews.apache.org/r/31443/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 31443: Accept dummy arguments for fs::mount().
Posted by Ian Downes <ia...@gmail.com>.
> On Feb. 26, 2015, 12:48 p.m., Jie Yu wrote:
> > src/linux/fs.cpp, line 181
> > <https://reviews.apache.org/r/31443/diff/1/?file=876476#file876476line181>
> >
> > Ditto here. No need to print 'target'.
Left as original code for this review.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31443/#review74359
-----------------------------------------------------------
On Feb. 25, 2015, 2:31 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31443/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2015, 2:31 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Accept dummy arguments for fs::mount().
>
>
> Diffs
> -----
>
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
>
> Diff: https://reviews.apache.org/r/31443/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 31443: Accept dummy arguments for fs::mount().
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31443/#review74359
-----------------------------------------------------------
Ship it!
src/linux/fs.hpp
<https://reviews.apache.org/r/31443/#comment120926>
Can you also adjust the comment here because the source can be a directory as well (in the case of bind mount).
src/linux/fs.hpp
<https://reviews.apache.org/r/31443/#comment120927>
I guess you need to explain what does a "dummy argument" mean here. It gets a little confusing initially and I need to look into documents to find out what does that mean.
Maybe say that source is optional. If not specified, it means xxx.
src/linux/fs.hpp
<https://reviews.apache.org/r/31443/#comment120928>
Ditto above. Maybe just say that file system type is optional.
Also, can you do a sweep in the code base to replace "none" with None()? I think we did use "none" in some cases for bind mounts.
src/linux/fs.cpp
<https://reviews.apache.org/r/31443/#comment120929>
I think we can just return ErrnoError() here. It's caller's responsibility to print all arguments.
src/linux/fs.cpp
<https://reviews.apache.org/r/31443/#comment120930>
Ditto here. No need to print 'target'.
- Jie Yu
On Feb. 25, 2015, 10:31 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31443/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2015, 10:31 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Accept dummy arguments for fs::mount().
>
>
> Diffs
> -----
>
> src/linux/fs.hpp ac8b5f416dae0a9388a3839b6f078abdcd42edae
> src/linux/fs.cpp b01d14c3a1b296566b2b17a7f540097bd7cc53dd
>
> Diff: https://reviews.apache.org/r/31443/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>