mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiang Yan Xu" <...@jxu.me>
Subject Re: Review Request 34138: AppC hash computation.
Date Tue, 07 Jul 2015 22:56:22 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message