You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/05/13 02:47:55 UTC
Review Request 34138: AppC hash computation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34138/
-----------------------------------------------------------
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.
Posted by Paul Brett <pa...@twopensource.com>.
> On May 13, 2015, 4:30 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc/hash.hpp, line 50
> > <https://reviews.apache.org/r/34138/diff/1/?file=957269#file957269line50>
> >
> > Not sure if this is generally available, but just checked on my OSX laptop and it's not there.
> >
> > shasum however is available, I wonder if it's possible to check if any of them is available?
How about "openssl dgst -sha512"?
- Paul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34138/#review83557
-----------------------------------------------------------
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
>
>
Re: Review Request 34138: AppC hash computation.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34138/#review83557
-----------------------------------------------------------
src/slave/containerizer/provisioners/appc/hash.hpp
<https://reviews.apache.org/r/34138/#comment134560>
Not sure if this is generally available, but just checked on my OSX laptop and it's not there.
shasum however is available, I wonder if it's possible to check if any of them is available?
src/slave/containerizer/provisioners/appc/hash.hpp
<https://reviews.apache.org/r/34138/#comment134559>
WSTRINGIFY to print the exit code?
- Timothy Chen
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
>
>
Re: Review Request 34138: AppC hash computation.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> 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.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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.
Posted by Vinod Kone <vi...@gmail.com>.
> 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.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34138/
-----------------------------------------------------------
(Updated June 22, 2015, 9:46 a.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Improve multiplatform support: check a few alternatives to find something to compute a sha-512 file hash.
Repository: mesos
Description
-------
AppC hash computation.
Diffs (updated)
-----
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.
Posted by Ian Downes <ia...@gmail.com>.
> 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.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
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
>
>