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