You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2015/02/26 18:53:49 UTC
Review Request 31489: Fix use of buffers in OS to remove leaks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/
-----------------------------------------------------------
Review request for mesos, Joerg Schad and Till Toenshoff.
Bugs: MESOS-2412
https://issues.apache.org/jira/browse/MESOS-2412
Repository: mesos
Description
-------
Fix use of buffers in OS to remove leaks.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
Diff: https://reviews.apache.org/r/31489/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74327
-----------------------------------------------------------
Patch looks great!
Reviews applied: [31489]
All tests passed.
- Mesos ReviewBot
On Feb. 26, 2015, 6:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:14 p.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74307
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On Feb. 26, 2015, 6:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:14 p.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74316
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rojas
On Feb. 26, 2015, 7:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 7:14 p.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 26, 2015, 6:14 p.m., Ben Mahler wrote:
> > This is pretty inconsistent with the rest of our code. Why can't you add the missing deletes?
>
> Dominic Hamon wrote:
> we can. but why not start getting rid of the archaic practice of using raw pointers?
>
> Timothy Chen wrote:
> I agree having less prone to errors is more desired.
>From the title, it looks like this would have changed all buffers in os.hpp, rather than introducing inconsistency in the process of fixing the two leaks (detected by coverity).
More importantly, is '`vector`' an intuitive way to capture a fixed size buffer on the heap? I'm all for improving this, but we need to think more carefully about how to represent buffers, do we want to introduce some kind of '`Buffer`' abstraction if benefical to readability (e.g. implicitly castable to the pointer type, etc), or consider `Owned<>`, `unique_ptr`, `scoped_ptr`, etc? Let's do these kind of changes independently.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74305
-----------------------------------------------------------
On Feb. 26, 2015, 6:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:14 p.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Timothy Chen <tn...@apache.org>.
> On Feb. 26, 2015, 6:14 p.m., Ben Mahler wrote:
> > This is pretty inconsistent with the rest of our code. Why can't you add the missing deletes?
>
> Dominic Hamon wrote:
> we can. but why not start getting rid of the archaic practice of using raw pointers?
I agree having less prone to errors is more desired.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74305
-----------------------------------------------------------
On Feb. 26, 2015, 6:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:14 p.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Feb. 26, 2015, 10:14 a.m., Ben Mahler wrote:
> > This is pretty inconsistent with the rest of our code. Why can't you add the missing deletes?
we can. but why not start getting rid of the archaic practice of using raw pointers?
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74305
-----------------------------------------------------------
On Feb. 26, 2015, 10:14 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 10:14 a.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Joerg Schad <jo...@mesosphere.io>.
> On Feb. 26, 2015, 6:14 p.m., Ben Mahler wrote:
> > This is pretty inconsistent with the rest of our code. Why can't you add the missing deletes?
>
> Dominic Hamon wrote:
> we can. but why not start getting rid of the archaic practice of using raw pointers?
>
> Timothy Chen wrote:
> I agree having less prone to errors is more desired.
>
> Ben Mahler wrote:
> From the title, it looks like this would have changed all buffers in os.hpp, rather than introducing inconsistency in the process of fixing the two leaks (detected by coverity).
>
> More importantly, is '`vector`' an intuitive way to capture a fixed size buffer on the heap? I'm all for improving this, but we need to think more carefully about how to represent buffers, do we want to introduce some kind of '`Buffer`' abstraction if benefical to readability (e.g. implicitly castable to the pointer type, etc), or consider `Owned<>`, `unique_ptr`, `scoped_ptr`, etc? Let's do these kind of changes independently.
I must say that I don't like the use of raw pointers here (and agree with Dominic, Timothy and Alexander on this), but Ben is correct is pointing out that these are two independent changes: removing the memleak and introducing a better(TBD) datastructure.
As I actually had a patch just introducing the two delete[] ready this morning (but then Dominic was faster :-) ) I just that one to RB here: https://reviews.apache.org/r/31494/ .
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74305
-----------------------------------------------------------
On Feb. 26, 2015, 6:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:14 p.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74305
-----------------------------------------------------------
This is pretty inconsistent with the rest of our code. Why can't you add the missing deletes?
- Ben Mahler
On Feb. 26, 2015, 6:14 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:14 p.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/
-----------------------------------------------------------
(Updated Feb. 26, 2015, 10:14 a.m.)
Review request for mesos, Joerg Schad and Till Toenshoff.
Bugs: MESOS-2412
https://issues.apache.org/jira/browse/MESOS-2412
Repository: mesos
Description
-------
Fix use of buffers in OS to remove leaks.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
Diff: https://reviews.apache.org/r/31489/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 31489: Fix use of buffers in OS to remove leaks.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31489/#review74303
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/31489/#comment120858>
buffer.data()?
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/31489/#comment120856>
ditto
- Ian Downes
On Feb. 26, 2015, 9:53 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31489/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 9:53 a.m.)
>
>
> Review request for mesos, Joerg Schad and Till Toenshoff.
>
>
> Bugs: MESOS-2412
> https://issues.apache.org/jira/browse/MESOS-2412
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix use of buffers in OS to remove leaks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 8a4fda97ee29c185471a69f60803efc193cbe922
>
> Diff: https://reviews.apache.org/r/31489/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>