Re: Review Request 54291: Added os::ptsname to stout.

[Benjamin Bannier]

> On Dec. 2, 2016, 10:16 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/posix/os.hpp, line 480
> > 
> >
> > We should be able to use a simple function-local static here and avoid 
> > the global leak of `mutex`, e.g.,
> > 
> > static std::mutex mutex;
> 
> Jie Yu wrote:
> We use pointer here to avoid global non-POD data.

This sounds like a very useful additon to the coding standard. Currently the 
code base seems to be split regarding the right pattern, e.g., overly 
simplistic grep patterns give

3rdparty/libprocess/src/clock.cpp:static recursive_mutex* timers_mutex = 
new recursive_mutex();
3rdparty/libprocess/src/libevent.cpp:static std::mutex* functions_mutex = 
new std::mutex();
3rdparty/libprocess/src/process.cpp:  static std::mutex* prefixes_mutex = 
new std::mutex();
3rdparty/libprocess/src/process.cpp:static std::mutex* socket_mutex = new 
std::mutex();
3rdparty/libprocess/src/process.cpp:static std::recursive_mutex* 
filterer_mutex = new std::recursive_mutex();
3rdparty/stout/include/stout/posix/os.hpp:  static std::mutex* mutex = new 
std::mutex;

and

src/authentication/cram_md5/auxprop.hpp:  static std::mutex mutex;
src/hook/manager.cpp:static std::mutex mutex;
src/linux/fs.cpp:static std::mutex mutex;
src/module/manager.hpp:  static std::mutex mutex;
src/tests/containerizer.cpp:static std::mutex mutex;


- Benjamin


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


On Dec. 5, 2016, 6:34 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 5, 2016, 6:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54291: Added os::ptsname to stout.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:34 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Addressed comments. Rebased.


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


Repository: mesos


Description
---

Added os::ptsname to stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54291: Added os::ptsname to stout.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54291: Added os::ptsname to stout.

[Benjamin Hindman]

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


Ship it!




Adding the TODO for `ptsname_r` on Linux SGTM as well as using `const char*` if 
possible. Otherwise LGTM.

- Benjamin Hindman


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54291: Added os::ptsname to stout.

[Jie Yu]

> On Dec. 2, 2016, 9:16 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/posix/os.hpp, line 480
> > 
> >
> > We should be able to use a simple function-local static here and avoid 
> > the global leak of `mutex`, e.g.,
> > 
> > static std::mutex mutex;

We use pointer here to avoid global non-POD data.


- Jie


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


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54291: Added os::ptsname to stout.

[Kevin Klues]

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




3rdparty/stout/include/stout/posix/os.hpp (line 480)


If I recall, there was a reason we usually only  declare static pointers 
(and leak memory) instead of statically declaring actual objects, but I don't 
remember of the top of my head. Joris explained it to me once.


- Kevin Klues


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54291: Added os::ptsname to stout.

[Jie Yu]

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

(Updated Dec. 2, 2016, 6:29 p.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


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


Repository: mesos


Description
---

Added os::ptsname to stout.


Diffs
-

  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54291: Added os::ptsname to stout.

[Benjamin Bannier]

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




3rdparty/stout/include/stout/posix/os.hpp (line 480)


We should be able to use a simple function-local static here and avoid the 
global leak of `mutex`, e.g.,

static std::mutex mutex;



3rdparty/stout/include/stout/posix/os.hpp (line 483)


We can use a `const char*` here as we wouldn't want to modify the returned 
value.


- Benjamin Bannier


On Dec. 2, 2016, 7:42 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 2, 2016, 7:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 54291: Added os::ptsname to stout.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Kevin Klues.


Repository: mesos


Description
---

Added os::ptsname to stout.


Diffs
-

  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 

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


Testing
---

make check


Thanks,

Jie Yu