Re: Review Request 34138: AppC hash computation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review141466 --- Closing this review due to inactivity. Please see our [guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md) for reopening reviews. - Joris Van Remoortere On July 7, 2015, 7:42 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34138/ > --- > > (Updated July 7, 2015, 7:42 p.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > AppC hash computation. > > > Diffs > - > > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/34138/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34138: AppC hash computation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review91145 --- src/slave/containerizer/provisioners/appc/hash.hpp (line 40) https://reviews.apache.org/r/34138/#comment144424 A class with only static methods seems weird. Why not put this in a namespace instead? Also, why is this defined here and not in libprocess? src/slave/containerizer/provisioners/appc/hash.hpp (line 61) https://reviews.apache.org/r/34138/#comment144423 This should take a 'Path' type instead of 'string'. src/slave/containerizer/provisioners/appc/hash.hpp (line 69) https://reviews.apache.org/r/34138/#comment144425 looks like 'command()' is only used once here. why split it into a function? - Vinod Kone On July 7, 2015, 7:42 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 7:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34138: AppC hash computation.
On July 9, 2015, 6:47 p.m., Vinod Kone wrote: Also, we need tests!? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review91145 --- On July 7, 2015, 7:42 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 7:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34138: AppC hash computation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34138: AppC hash computation.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34138/#review90791
---
1. Agree that this is useful as a utility in libprocess. Not much overhead to
move it over right?
2. It feels like something that could be exposed as a function rather than
class, maybe a TODO.
src/slave/containerizer/provisioners/appc/hash.hpp (lines 22 - 31)
https://reviews.apache.org/r/34138/#comment143922
```
#include string
#include vector
#include stout/...
#include process/...
...
```
According to the style guide.
src/slave/containerizer/provisioners/appc/hash.hpp (lines 45 - 55)
https://reviews.apache.org/r/34138/#comment143917
I would do
```
if (!system(sha512sum -h /dev/null)) {...}
```
to avoid fixing the binary location.
src/slave/containerizer/provisioners/appc/hash.hpp (lines 89 - 91)
https://reviews.apache.org/r/34138/#comment143948
I think we use 4 spaces to continue a left angle bracket the same way we
continue an left paren.
src/slave/containerizer/provisioners/appc/hash.hpp (line 97)
https://reviews.apache.org/r/34138/#comment143939
The `command` is not necessarily `sha512sum`. Maybe use it a static member
so we detect once and use it with every hash() invocation?
src/slave/containerizer/provisioners/appc/hash.hpp (line 98)
https://reviews.apache.org/r/34138/#comment143940
Misaligned indentation.
src/slave/containerizer/provisioners/appc/hash.hpp (line 102)
https://reviews.apache.org/r/34138/#comment143949
The `command` is not necessarily `sha512sum`.
src/slave/containerizer/provisioners/appc/hash.hpp (line 109)
https://reviews.apache.org/r/34138/#comment143941
Misaligned indentation.
src/slave/containerizer/provisioners/appc/hash.hpp (lines 112 - 122)
https://reviews.apache.org/r/34138/#comment143946
This check doesn't work with openssl.
```
/usr/bin/openssl dgst -sha512 somefile.txt
SHA512(somefile.txt)=
5a73e55fd845981be5d5b87039c678b87404405d5d054c579cf684a18893d181085b9afde535c034221f858d2bcc2b14978b4d5f4d6facfaa1f81e727a010f3c
```
I think we only need shasum and sha512sum to cover both Linux and OSX.
- Jiang Yan Xu
On July 7, 2015, 12:42 p.m., Ian Downes wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34138/
---
(Updated July 7, 2015, 12:42 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Repository: mesos
Description
---
AppC hash computation.
Diffs
-
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/34138/diff/
Testing
---
Thanks,
Ian Downes
Re: Review Request 34138: AppC hash computation.
On July 7, 2015, 3:56 p.m., Jiang Yan Xu wrote: 1. Agree that this is useful as a utility in libprocess. Not much overhead to move it over right? 2. It feels like something that could be exposed as a function rather than class, maybe a TODO. OK I realized that doing the aforementioned refactorings is not as simple as moving the file so probably punting it is the right thing to do for now. 1. As a generic utility it's probably giong to be SHA instead SHA512. 2. OK SHA512::hash() is already static but I meant if made more generic like SHA(alorightm).hash() then shorthand functions like `sha1()`, `sha512()` is easier to use. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review90791 --- On July 7, 2015, 12:42 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34138: AppC hash computation.
On May 18, 2015, 4:37 p.m., Chi Zhang wrote: push the implementation down to stout? is it possible to swap to use devel packages for hashing in the future? Not to stout because it's asynchronous but perhaps to libprocess. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review84195 --- On May 12, 2015, 5:47 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated May 12, 2015, 5:47 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34138: AppC hash computation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review84195 --- push the implementation down to stout? is it possible to swap to use devel packages for hashing in the future? - Chi Zhang On May 13, 2015, 12:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated May 13, 2015, 12:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
