Re: Review Request 38747: Adding digest utilities

[Joris Van Remoortere]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review148876
---



Closing this review as Benjamin has followed up with separate reviews. Please 
see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On June 26, 2016, 8:23 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated June 26, 2016, 8:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added templatized implementations for digests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 404a7438e7cd53d9295fe6025b5af5fb83df939b 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> 99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Nov. 9, 2015, 8:17 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, Michael Park, and Timothy 
Chen.


Changes
---

adding mpark as reviewer


Repository: mesos


Description (updated)
---

Added templatized implementations for digests.


Diffs
-

  3rdparty/libprocess/Makefile.am 404a7438e7cd53d9295fe6025b5af5fb83df939b 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Nov. 5, 2015, 7:32 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

removed dependency


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs
-

  3rdparty/libprocess/Makefile.am 404a7438e7cd53d9295fe6025b5af5fb83df939b 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Nov. 5, 2015, 4:04 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

rebased


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 404a7438e7cd53d9295fe6025b5af5fb83df939b 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Nov. 1, 2015, 3:38 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

rebased


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 404a7438e7cd53d9295fe6025b5af5fb83df939b 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Oct. 23, 2015, 4:56 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

Added support for more digests


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Oct. 16, 2015, 1:45 a.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

Added std::async for async dispatch


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Oct. 15, 2015, 9:24 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

reverted back to async loop; fixed read size.


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Oct. 13, 2015, 12:54 a.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, line 71
> > 
> >
> > I imagine `fn` is expensive, after all, it's  cryptography. :)
> > 
> > Serially doing read and compute seems to be suboptimal but I simply 
> > parallelize read and compute can cause OOM when read is faster than 
> > compute. Of couse this is a classic producer and consumer problem and we 
> > can use a queue. We probably don't want to make things too complicated here 
> > in a single review but again, this is the issue of reimplemting a generic 
> > and widely used utility: we don't get to leverage state of the art for 
> > free...
> > 
> > Just a comment.

Added comment on preliminary performance test.


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review102302
---


On Oct. 13, 2015, 7:21 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Oct. 13, 2015, 7:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Oct. 13, 2015, 7:21 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Oct. 13, 2015, 12:54 a.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, lines 85-120
> > 
> >
> > These templates (here and below) have different levels of 
> > specializations, traits, typedefs in various places, which is hard to 
> > understand.
> > 
> > Do you think the following is simpler?
> > 
> > For each digest type, there is one **low-level** implementation, i.e., 
> > Init, Update and Final are called separatedly but they expose one common 
> > interface (without digest-type specific) arguments.
> > 
> > ```
> > class DigestImpl
> > {
> > public:
> >   void init() == 0; 
> >   int update(const void*, size_t) == 0; 
> >   int final(uint8_t*) == 0;
> >   static Try create(...);
> > };
> > ```
> > 
> > You have a low implementation for each type that inherits this 
> > interface and encapsulates its specific context variables in its member 
> > variables. The low-level implementation should be simply calling openssl 
> > APIs so it should be short and has no more redundant code among different 
> > implementations than specializations in different places and this 
> > consolidates handling of specific digest-types in one single place.
> > 
> > The high-level DigestUtil implementation or simply, the static generic 
> > method implementation can just create a low-level impl class and you can 
> > put common logic there.
> > 
> > What do you think?

I should probably try to reason about the choice of templates (as opposed to 
runtime polymorphism)

SSL digest API can be seen as "logic" and "structure". The logic is the same 
for all SHAs. Only structure is different. The difference in structure can be 
represented as template traits. This allows the difference between SHAs to be 
viewed as *declarative* (as opposed to *procedural*).

This has following advantages:
1. Any new SHA digest can be added by just adding the type traits 
*declaratively*. For example, to add SHA224, we just need to add the following 
lines:

```
template <> 
 
struct DigestTypeTraits 
 
{   
 
  typedef SHA224_CTX ctx_type;  
 
  static const size_t digest_length = SSHA224_DIGEST_LENGTH;
  
};

template <> 
 
DigestFunctionTraits::Init_fn   
 
  DigestFunctionTraits::Init = SHA224_Init; 
 

 

 
template <> 
 
DigestFunctionTraits::Update_fn 
 
  DigestFunctionTraits::Update = SHA224_Update; 
 

 

 
template <> 
 
DigestFunctionTraits::Final_fn  
 
  DigestFunctionTraits::Final = SHA224_Final; 
  
```

If you notice all of the above new code is declarative and does not add any new 
logic. This is possible because we have abstracted out the difference between 
SHA implementations into templates.

2. At the call site, clients can call digests simply by:

```
 DigestUtil::digest(string)

```

This avoids  runtime polymorphism.


> On Oct. 13, 2015, 12:54 a.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, lines 159-160
> > 
> >
> > About templatization. I previously commented that the implementation 
> > should be put in the CPP file.
> > 
> > You use templates but your interafce is not templatized and used only 
> > by this single component. Can't all the templates just be in the same 
> > source file as the caller so that the header only has the API declarations?
> > 
> > This of course becomes a moot point is we don't use template as I was 
> > suggesting above.

One of the reasons the templates are in header file is to allow client code 
like:

```
DigestUtil::digest(string)
```


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review102302
---


On Oct. 12, 2015, 9:14 p.m., Jojy Varghese wrote:
> 
> ---

Re: Review Request 38747: Adding digest utilities

[Jiang Yan Xu]

> On Oct. 1, 2015, 11:23 a.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a 
> > reference to a pending review  but 
> > didn't look at the review closely. I also have a ticket 
> > https://issues.apache.org/jira/browse/MESOS-3191 create before.
> > 
> > I have a few questions, just to help me understand the context.
> > 
> > 1) Since the motivation for this util is for computing and verifying docker 
> > image hash, how will we use it? Given that this implementation requires 
> > USE_SSL_SOCKET, do we want to tie the docker provisioner to the 
> > --enable-ssl flag? Or should we create another flag or make `libssl-dev` or 
> > `openssl-devel` a dependency by default? What is the plan?
> > 
> > 2) What's the advantage of using openssl APIs directly? This implementation 
> > is obviously more complex than the alternative 
> > , which simply calls out to a few 
> > widely distributed system binaries on our supported platforms. (sha256sum 
> > sha512sum are part of GNU coreutils while shasum is on every mac). The 
> > linked review needs to address some comments but it's not far from ready 
> > for shipit (it's not a priority for us right now but you can take it if you 
> > like).
> > 
> > Thanks!
> 
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
> 
> - We currently depend on SSL for docker remote registry client as thats 
> the authentication type we support. The code exposes simple APIs that can be 
> called to get digest of a string, file etc. The test file serves as examples 
> of usage.
> - The advantage is that we can use the API without spawning a process. 
> Else you can image the number of spawns for a docker image with many layers. 
> In short efficiency. Another reason is that the review you pointed me to also 
> depends on those utils to be available. So we still need a fallback when 
> those utils are not available.
> - The code is open to adding more fallback implementations that can 
> incorporate the method in https://reviews.apache.org/r/34138/. So we can 
> still add those fallback. I would see this implementation as a superset and 
> not a replacement for the code presented in 
> https://reviews.apache.org/r/34138/.
> 
> -jojy
> 
> Jiang Yan Xu wrote:
> Sorry for the delay due to my travel. I realize much work has already 
> been done in this and we should probably still push this forward so I'll 
> comment on the code separately but the following is still high-level:
> 
> I guess I am still not fully sold on the necessity of this when we 
> already do the rest of the image provisioning via subprocess commands. (i.e., 
> `cp`, `tar`). Hashing using these commands look natual to me, especially 
> because it doesn't interact with the rest of the codebase in anyway. It maybe 
> something easier to do for the task at hand. Some of the thoughts below are 
> applicable if we adopt the native implementation as well.
> 
> ## SSL
> Ok I see that SSL is required for for docker (or more precicsely docker 
> when pulling from the registry). But how are we enabling this dependency? 
> IIUC currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which 
> has nothing to do with SHA. I can understand that in order to use docker 
> registry I have to turn on libevent + SSL but if I just want to use the 
> hashing utility I have think I should be forced to switch from libev to 
> libevent. Maybe we should come up with another flag and preprocessor macro 
> for this (use openssl headers). FWIW I think openssl may have expected common 
> usage in the future to be a hard/unconditional dependency. Which is easier?
> 
> I am all for exposing simple and generic APIs for this utility but I 
> think we are discussing the implementation details here.
> 
> ## Overhead of spawning a process
> We already call `tar` and perhaps `cp` in subprocesses for each layer so 
> this is definitely not a bottleneck. :)
> 
> ## Dependency on the availability of the binary
> See my comments above. I think sha256sum and sha512sum on Linux and 
> shasum on OSX are widely distributed (more common than the openssl headers). 
> They appear to be as common as `tar` and `mount`.
> 
> ## Subprocess as a fallback.
> Sure they can coexist. I am merely talking about complexity and priority. 
> If openssl if a hard dependency then the subprocess implementation probably 
> doesn't have to exist. On the other hand, this native implementation does 
> data reading and processing in a serial manner, I don't know if these 
> binaries have any optimization. Note that for large binaries we are using 
> SHAing takes close to a minute so any optimization would be great to have.
> 
> Jojy Varghese wrote:
> Thanks for the comments Yan, inspite of your busy schedule.
> 
> To summarize the "YEAs"

Re: Review Request 38747: Adding digest utilities

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review102302
---



3rdparty/libprocess/include/process/digest.hpp (line 39)


Kill the line here.



3rdparty/libprocess/include/process/digest.hpp (line 71)


I imagine `fn` is expensive, after all, it's  cryptography. :)

Serially doing read and compute seems to be suboptimal but I simply 
parallelize read and compute can cause OOM when read is faster than compute. Of 
couse this is a classic producer and consumer problem and we can use a queue. 
We probably don't want to make things too complicated here in a single review 
but again, this is the issue of reimplemting a generic and widely used utility: 
we don't get to leverage state of the art for free...

Just a comment.



3rdparty/libprocess/include/process/digest.hpp (line 84)


No space before and after the conditional guard. Here and elsewhere.



3rdparty/libprocess/include/process/digest.hpp (lines 85 - 120)


These templates (here and below) have different levels of specializations, 
traits, typedefs in various places, which is hard to understand.

Do you think the following is simpler?

For each digest type, there is one **low-level** implementation, i.e., 
Init, Update and Final are called separatedly but they expose one common 
interface (without digest-type specific) arguments.

```
class DigestImpl
{
public:
  void init() == 0; 
  int update(const void*, size_t) == 0; 
  int final(uint8_t*) == 0;
  static Try create(...);
};
```

You have a low implementation for each type that inherits this interface 
and encapsulates its specific context variables in its member variables. The 
low-level implementation should be simply calling openssl APIs so it should be 
short and has no more redundant code among different implementations than 
specializations in different places and this consolidates handling of specific 
digest-types in one single place.

The high-level DigestUtil implementation or simply, the static generic 
method implementation can just create a low-level impl class and you can put 
common logic there.

What do you think?



3rdparty/libprocess/include/process/digest.hpp (line 125)


No indentation in a new line in this case, here and elsewhere.



3rdparty/libprocess/include/process/digest.hpp (lines 159 - 160)


About templatization. I previously commented that the implementation should 
be put in the CPP file.

You use templates but your interafce is not templatized and used only by 
this single component. Can't all the templates just be in the same source file 
as the caller so that the header only has the API declarations?

This of course becomes a moot point is we don't use template as I was 
suggesting above.


- Jiang Yan Xu


On Oct. 12, 2015, 2:14 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Oct. 12, 2015, 2:14 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, line 61
> > 
> >
> > Consider using `boost::shared_array data` (see io.cpp).

Since c++11 standard way is to use shared_ptr with deleter, shared_ptr is used. 
I have changed the deleter to use the std default deleter. I am not passing 
smart pointer to the called function as its not required.


> On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, lines 73-75
> > 
> >
> > What does this mean? EOF? Add a comment?
> > 
> > Also, we don't need to special case this right? It will terminate at 
> > the next iteration right?

You are right. The early return is just an optimization.


> On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, line 299
> > 
> >
> > Would it be safer to just have the caller select from the `Digest` enum?

This API was chosen to enable use cases like Docker which gets the SHA method 
as a string. This reduces the logic at client/caller site.


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101451
---


On Oct. 12, 2015, 9:14 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Oct. 12, 2015, 9:14 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Oct. 12, 2015, 9:14 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

review comments adderssed.


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Oct. 1, 2015, 6:23 p.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a 
> > reference to a pending review  but 
> > didn't look at the review closely. I also have a ticket 
> > https://issues.apache.org/jira/browse/MESOS-3191 create before.
> > 
> > I have a few questions, just to help me understand the context.
> > 
> > 1) Since the motivation for this util is for computing and verifying docker 
> > image hash, how will we use it? Given that this implementation requires 
> > USE_SSL_SOCKET, do we want to tie the docker provisioner to the 
> > --enable-ssl flag? Or should we create another flag or make `libssl-dev` or 
> > `openssl-devel` a dependency by default? What is the plan?
> > 
> > 2) What's the advantage of using openssl APIs directly? This implementation 
> > is obviously more complex than the alternative 
> > , which simply calls out to a few 
> > widely distributed system binaries on our supported platforms. (sha256sum 
> > sha512sum are part of GNU coreutils while shasum is on every mac). The 
> > linked review needs to address some comments but it's not far from ready 
> > for shipit (it's not a priority for us right now but you can take it if you 
> > like).
> > 
> > Thanks!
> 
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
> 
> - We currently depend on SSL for docker remote registry client as thats 
> the authentication type we support. The code exposes simple APIs that can be 
> called to get digest of a string, file etc. The test file serves as examples 
> of usage.
> - The advantage is that we can use the API without spawning a process. 
> Else you can image the number of spawns for a docker image with many layers. 
> In short efficiency. Another reason is that the review you pointed me to also 
> depends on those utils to be available. So we still need a fallback when 
> those utils are not available.
> - The code is open to adding more fallback implementations that can 
> incorporate the method in https://reviews.apache.org/r/34138/. So we can 
> still add those fallback. I would see this implementation as a superset and 
> not a replacement for the code presented in 
> https://reviews.apache.org/r/34138/.
> 
> -jojy
> 
> Jiang Yan Xu wrote:
> Sorry for the delay due to my travel. I realize much work has already 
> been done in this and we should probably still push this forward so I'll 
> comment on the code separately but the following is still high-level:
> 
> I guess I am still not fully sold on the necessity of this when we 
> already do the rest of the image provisioning via subprocess commands. (i.e., 
> `cp`, `tar`). Hashing using these commands look natual to me, especially 
> because it doesn't interact with the rest of the codebase in anyway. It maybe 
> something easier to do for the task at hand. Some of the thoughts below are 
> applicable if we adopt the native implementation as well.
> 
> ## SSL
> Ok I see that SSL is required for for docker (or more precicsely docker 
> when pulling from the registry). But how are we enabling this dependency? 
> IIUC currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which 
> has nothing to do with SHA. I can understand that in order to use docker 
> registry I have to turn on libevent + SSL but if I just want to use the 
> hashing utility I have think I should be forced to switch from libev to 
> libevent. Maybe we should come up with another flag and preprocessor macro 
> for this (use openssl headers). FWIW I think openssl may have expected common 
> usage in the future to be a hard/unconditional dependency. Which is easier?
> 
> I am all for exposing simple and generic APIs for this utility but I 
> think we are discussing the implementation details here.
> 
> ## Overhead of spawning a process
> We already call `tar` and perhaps `cp` in subprocesses for each layer so 
> this is definitely not a bottleneck. :)
> 
> ## Dependency on the availability of the binary
> See my comments above. I think sha256sum and sha512sum on Linux and 
> shasum on OSX are widely distributed (more common than the openssl headers). 
> They appear to be as common as `tar` and `mount`.
> 
> ## Subprocess as a fallback.
> Sure they can coexist. I am merely talking about complexity and priority. 
> If openssl if a hard dependency then the subprocess implementation probably 
> doesn't have to exist. On the other hand, this native implementation does 
> data reading and processing in a serial manner, I don't know if these 
> binaries have any optimization. Note that for large binaries we are using 
> SHAing takes close to a minute so any optimization would be great to have.

Thanks for the comments Yan, inspite of your busy schedule.

To summarize the "YEAs" and "NAYs" for this solution:

YEA:
- Finer

Re: Review Request 38747: Adding digest utilities

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101451
---


Incomplete review on v13. Continuing on the latest version.


3rdparty/libprocess/include/process/digest.hpp (line 40)


`/**` for doxygen comments.



3rdparty/libprocess/include/process/digest.hpp (line 59)


Relying on templates and traits or not, these should not be exposed in the 
same place where the "public" methods are seen. Put then in a "internal" 
namespace is a conventional practice.



3rdparty/libprocess/include/process/digest.hpp (line 61)


Consider using `boost::shared_array data` (see io.cpp).



3rdparty/libprocess/include/process/digest.hpp (line 66)


No need to start a new line for a single argument.



3rdparty/libprocess/include/process/digest.hpp (line 67)


With `then()` you can get `size_t` result directly so no need for taking a 
future here.



3rdparty/libprocess/include/process/digest.hpp (lines 73 - 75)


What does this mean? EOF? Add a comment?

Also, we don't need to special case this right? It will terminate at the 
next iteration right?



3rdparty/libprocess/include/process/digest.hpp (line 299)


Would it be safer to just have the caller select from the `Digest` enum?


- Jiang Yan Xu


On Oct. 7, 2015, 7:37 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Oct. 7, 2015, 7:37 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jiang Yan Xu]

> On Oct. 1, 2015, 11:23 a.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a 
> > reference to a pending review  but 
> > didn't look at the review closely. I also have a ticket 
> > https://issues.apache.org/jira/browse/MESOS-3191 create before.
> > 
> > I have a few questions, just to help me understand the context.
> > 
> > 1) Since the motivation for this util is for computing and verifying docker 
> > image hash, how will we use it? Given that this implementation requires 
> > USE_SSL_SOCKET, do we want to tie the docker provisioner to the 
> > --enable-ssl flag? Or should we create another flag or make `libssl-dev` or 
> > `openssl-devel` a dependency by default? What is the plan?
> > 
> > 2) What's the advantage of using openssl APIs directly? This implementation 
> > is obviously more complex than the alternative 
> > , which simply calls out to a few 
> > widely distributed system binaries on our supported platforms. (sha256sum 
> > sha512sum are part of GNU coreutils while shasum is on every mac). The 
> > linked review needs to address some comments but it's not far from ready 
> > for shipit (it's not a priority for us right now but you can take it if you 
> > like).
> > 
> > Thanks!
> 
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
> 
> - We currently depend on SSL for docker remote registry client as thats 
> the authentication type we support. The code exposes simple APIs that can be 
> called to get digest of a string, file etc. The test file serves as examples 
> of usage.
> - The advantage is that we can use the API without spawning a process. 
> Else you can image the number of spawns for a docker image with many layers. 
> In short efficiency. Another reason is that the review you pointed me to also 
> depends on those utils to be available. So we still need a fallback when 
> those utils are not available.
> - The code is open to adding more fallback implementations that can 
> incorporate the method in https://reviews.apache.org/r/34138/. So we can 
> still add those fallback. I would see this implementation as a superset and 
> not a replacement for the code presented in 
> https://reviews.apache.org/r/34138/.
> 
> -jojy

Sorry for the delay due to my travel. I realize much work has already been done 
in this and we should probably still push this forward so I'll comment on the 
code separately but the following is still high-level:

I guess I am still not fully sold on the necessity of this when we already do 
the rest of the image provisioning via subprocess commands. (i.e., `cp`, 
`tar`). Hashing using these commands look natual to me, especially because it 
doesn't interact with the rest of the codebase in anyway. It maybe something 
easier to do for the task at hand. Some of the thoughts below are applicable if 
we adopt the native implementation as well.

## SSL
Ok I see that SSL is required for for docker (or more precicsely docker when 
pulling from the registry). But how are we enabling this dependency? IIUC 
currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which has 
nothing to do with SHA. I can understand that in order to use docker registry I 
have to turn on libevent + SSL but if I just want to use the hashing utility I 
have think I should be forced to switch from libev to libevent. Maybe we should 
come up with another flag and preprocessor macro for this (use openssl 
headers). FWIW I think openssl may have expected common usage in the future to 
be a hard/unconditional dependency. Which is easier?

I am all for exposing simple and generic APIs for this utility but I think we 
are discussing the implementation details here.

## Overhead of spawning a process
We already call `tar` and perhaps `cp` in subprocesses for each layer so this 
is definitely not a bottleneck. :)

## Dependency on the availability of the binary
See my comments above. I think sha256sum and sha512sum on Linux and shasum on 
OSX are widely distributed (more common than the openssl headers). They appear 
to be as common as `tar` and `mount`.

## Subprocess as a fallback.
Sure they can coexist. I am merely talking about complexity and priority. If 
openssl if a hard dependency then the subprocess implementation probably 
doesn't have to exist. On the other hand, this native implementation does data 
reading and processing in a serial manner, I don't know if these binaries have 
any optimization. Note that for large binaries we are using SHAing takes close 
to a minute so any optimization would be great to have.


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101257
---


On Oct. 7, 2015, 7:37 p.m., Jojy Varghese wrote:
> 
> -

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Oct. 8, 2015, 2:37 a.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

replaced raw pointers with smart ptrs.


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Oct. 1, 2015, 6:23 p.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a 
> > reference to a pending review  but 
> > didn't look at the review closely. I also have a ticket 
> > https://issues.apache.org/jira/browse/MESOS-3191 create before.
> > 
> > I have a few questions, just to help me understand the context.
> > 
> > 1) Since the motivation for this util is for computing and verifying docker 
> > image hash, how will we use it? Given that this implementation requires 
> > USE_SSL_SOCKET, do we want to tie the docker provisioner to the 
> > --enable-ssl flag? Or should we create another flag or make `libssl-dev` or 
> > `openssl-devel` a dependency by default? What is the plan?
> > 
> > 2) What's the advantage of using openssl APIs directly? This implementation 
> > is obviously more complex than the alternative 
> > , which simply calls out to a few 
> > widely distributed system binaries on our supported platforms. (sha256sum 
> > sha512sum are part of GNU coreutils while shasum is on every mac). The 
> > linked review needs to address some comments but it's not far from ready 
> > for shipit (it's not a priority for us right now but you can take it if you 
> > like).
> > 
> > Thanks!

Thanks for feedback. To answer your questions:

- We currently depend on SSL for docker remote registry client as thats the 
authentication type we support. The code exposes simple APIs that can be called 
to get digest of a string, file etc. The test file serves as examples of usage.
- The advantage is that we can use the API without spawning a process. Else you 
can image the number of spawns for a docker image with many layers. In short 
efficiency. Another reason is that the review you pointed me to also depends on 
those utils to be available. So we still need a fallback when those utils are 
not available.
- The code is open to adding more fallback implementations that can incorporate 
the method in https://reviews.apache.org/r/34138/. So we can still add those 
fallback. I would see this implementation as a superset and not a replacement 
for the code presented in https://reviews.apache.org/r/34138/.

-jojy


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101257
---


On Sept. 30, 2015, 8:11 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 30, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101257
---


Sorry I haven't chimed in earlier. I made one comment earlier with a reference 
to a pending review  but didn't look at 
the review closely. I also have a ticket 
https://issues.apache.org/jira/browse/MESOS-3191 create before.

I have a few questions, just to help me understand the context.

1) Since the motivation for this util is for computing and verifying docker 
image hash, how will we use it? Given that this implementation requires 
USE_SSL_SOCKET, do we want to tie the docker provisioner to the --enable-ssl 
flag? Or should we create another flag or make `libssl-dev` or `openssl-devel` 
a dependency by default? What is the plan?

2) What's the advantage of using openssl APIs directly? This implementation is 
obviously more complex than the alternative 
, which simply calls out to a few widely 
distributed system binaries on our supported platforms. (sha256sum sha512sum 
are part of GNU coreutils while shasum is on every mac). The linked review 
needs to address some comments but it's not far from ready for shipit (it's not 
a priority for us right now but you can take it if you like).

Thanks!

- Jiang Yan Xu


On Sept. 30, 2015, 1:11 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 30, 2015, 1:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 30, 2015, 8:11 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

more tests; dangling pointer;


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 30, 2015, 6:50 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

added more tests; fixed possible dangling pointer.


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Ben Mahler]

> On Sept. 29, 2015, 9:35 p.m., Ben Mahler wrote:
> > Tim, could you also get a libprocess maintainer to review this?
> 
> Timothy Chen wrote:
> Volunteered you :)

I chatted with Yan, he'll be helping review this.


- Ben


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101026
---


On Sept. 29, 2015, 9:40 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 29, 2015, 9:40 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Sept. 29, 2015, 9:17 p.m., Timothy Chen wrote:
> > 3rdparty/libprocess/src/tests/digest_tests.cpp, line 58
> > 
> >
> > What's the motivation behind having both simple and dynamic 
> > verification?

"Simple" gives you the ability to call a digest (say SHA 256) directly if you 
already know the type. The dynamic part is so that the client code does not 
have to do the selection. Use cases like the docker registry where the sha type 
is dynamic, the client code will have to do the dynamic call. By keeping it 
central, this logic need not be repeated everywhere.


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101018
---


On Sept. 29, 2015, 9:40 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 29, 2015, 9:40 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Timothy Chen]

> On Sept. 29, 2015, 9:35 p.m., Ben Mahler wrote:
> > Tim, could you also get a libprocess maintainer to review this?

Volunteered you :)


- Timothy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101026
---


On Sept. 29, 2015, 6:41 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 29, 2015, 6:41 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101026
---


Tim, could you also get a libprocess maintainer to review this?

- Ben Mahler


On Sept. 29, 2015, 6:41 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 29, 2015, 6:41 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Timothy Chen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101018
---



3rdparty/libprocess/src/tests/digest_tests.cpp (line 58)


What's the motivation behind having both simple and dynamic verification?


- Timothy Chen


On Sept. 29, 2015, 6:41 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 29, 2015, 6:41 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 29, 2015, 6:41 p.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

style fixes.


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 29, 2015, 6:30 p.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

eliminated need for ifdef


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 29, 2015, 12:19 a.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

Review comments, fixed bug in test


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Neil Conway]

> On Sept. 28, 2015, 6:32 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, line 222
> > 
> >
> > Why do we initialize this to `{0}`?
> 
> Jojy Varghese wrote:
> initialization of auto variables

IMO this is a little misleading: the array is actually initialized by the for 
loop that follows below. I think it would be a bit clearer to omit the 
initializer, and then explicitly NUL-terminate the array after the for loop.


- Neil


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100835
---


On Sept. 28, 2015, 10:14 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 28, 2015, 10:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Sept. 28, 2015, 7:17 p.m., Jiang Yan Xu wrote:
> > High-level comments:
> > 
> > - This implementation relies on USE_SSL_SOCKET which is tied to 
> > `--enable_ssl`. Conceptually the two should be decoupled. Ian Downes has an 
> > implementation which uses the system commands that are widely distributed 
> > on *nix boxes: https://reviews.apache.org/r/34138/. Can such an 
> > implemenation be used as a fallback at least?
> > - Can the implentation be put in a cpp file and leave only public (the 
> > recommended way of using this utility) methods in the header (e.g. 
> > `process::verify(...)` and `process::digest()`)?

Thanks for the suggestion and history.
- digest.hpp's process::digest and process::verify has default implementations 
(when USE_SSL_SOCKET is not available). So thats the fallback for now.
- Since this implementation is dependent on using traits/templates, the 
implementation is in hpp file
- I can work on adding Ian's implementation in this patch or let Ian add it as 
an extension to this patch.


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100850
---


On Sept. 28, 2015, 10:14 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 28, 2015, 10:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 28, 2015, 10:14 p.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

review comments.


Repository: mesos


Description
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Sept. 28, 2015, 6:32 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, line 191
> > 
> >
> > I believe this leaks "fd".

good catch.


> On Sept. 28, 2015, 6:32 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, line 222
> > 
> >
> > Why do we initialize this to `{0}`?

initialization of auto variables


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100835
---


On Sept. 25, 2015, 8:33 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 8:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100850
---


High-level comments:

- This implementation relies on USE_SSL_SOCKET which is tied to `--enable_ssl`. 
Conceptually the two should be decoupled. Ian Downes has an implementation 
which uses the system commands that are widely distributed on *nix boxes: 
https://reviews.apache.org/r/34138/. Can such an implemenation be used as a 
fallback at least?
- Can the implentation be put in a cpp file and leave only public (the 
recommended way of using this utility) methods in the header (e.g. 
`process::verify(...)` and `process::digest()`)?

- Jiang Yan Xu


On Sept. 25, 2015, 1:33 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 1:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100839
---



3rdparty/libprocess/include/process/digest.hpp (lines 210 - 226)


digest printed out is different between digest(const char* bytes, size_t 
size) and digest(const std::string& filePath) with a corresponding same string. 

Logic check is needed. (proprably is a logical error in my test case)



3rdparty/libprocess/include/process/digest.hpp (line 275)


This line is supposed to be deleted.


- Gilbert Song


On Sept. 25, 2015, 8:33 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 8:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Neil Conway]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100835
---



3rdparty/libprocess/include/process/digest.hpp (line 191)


I believe this leaks "fd".



3rdparty/libprocess/include/process/digest.hpp (line 205)


Probably "constexpr" is better than "static const".



3rdparty/libprocess/include/process/digest.hpp (line 219)


Seems like this duplicates some code above: can we refactor this into a 
function?



3rdparty/libprocess/include/process/digest.hpp (line 222)


Why do we initialize this to `{0}`?



3rdparty/libprocess/include/process/digest.hpp (line 225)


Need a space after "for".

Also, is sprintf() really the best choice here? There's std::hex + 
std::stringstream, although maybe that's not better overall.



3rdparty/libprocess/include/process/digest.hpp (line 237)


Typo



3rdparty/libprocess/include/process/digest.hpp (line 265)


Typo



3rdparty/libprocess/include/process/digest.hpp (line 315)


Typo


- Neil Conway


On Sept. 25, 2015, 8:33 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 8:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100686
---


Patch looks great!

Reviews applied: [38443, 38579, 38580, 38747]

All tests passed.

- Mesos ReviewBot


On Sept. 25, 2015, 8:33 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 8:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256, SHA512 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 25, 2015, 8:33 p.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

build fix.


Repository: mesos


Description (updated)
---

Added SHA256, SHA512 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 25, 2015, 6:33 p.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

rebased with master


Repository: mesos


Description
---

Added SHA256 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100619
---


Bad patch!

Reviews applied: [38443, 38579]

Failed command: ./support/apply-review.sh -n -r 38579

Error:
 2015-09-25 17:21:42 URL:https://reviews.apache.org/r/38579/diff/raw/ 
[3389/3389] -> "38579.patch" [1]
error: patch failed: src/tests/containerizer/provisioner_docker_tests.cpp:48
error: src/tests/containerizer/provisioner_docker_tests.cpp: patch does not 
apply
Failed to apply patch

- Mesos ReviewBot


On Sept. 25, 2015, 4:45 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 4:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

> On Sept. 25, 2015, 6:24 a.m., Alex Clemmer wrote:
> > Hey, it doesn't look like `src/tests/digest_tests.cpp` is being added to 
> > the `CMakeLists.txt` file in `3rdparty/libprocess/src/tests`. Is that 
> > correct? If not, could we please add it there as well?
> > 
> > (I have forgotten to add a list of header files as dependencies, so you 
> > can't add that quite yet.)
> 
> Alex Clemmer wrote:
> Ah, after testing, we won't have to include the header lists at all, so 
> you don't have to add that to a list. It turns out that if you pass the 
> `include/` folder into the `include_directory` function, it automatically 
> will monitor those files for changes. :) So we're good.
> 
> But, unless I'm mistaken, we'd still have to add that test to the 
> `CMakeLists.txt`. Let me know if my thinking is wrong here.

Added. Didnt know this extra step existed :)


- Jojy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100539
---


On Sept. 25, 2015, 4:45 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 4:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 25, 2015, 4:45 p.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

addressed review comments


Repository: mesos


Description
---

Added SHA256 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 25, 2015, 3:23 p.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

Chnged implementation to make it generic.


Repository: mesos


Description
---

Added SHA256 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Alex Clemmer]

> On Sept. 25, 2015, 6:24 a.m., Alex Clemmer wrote:
> > Hey, it doesn't look like `src/tests/digest_tests.cpp` is being added to 
> > the `CMakeLists.txt` file in `3rdparty/libprocess/src/tests`. Is that 
> > correct? If not, could we please add it there as well?
> > 
> > (I have forgotten to add a list of header files as dependencies, so you 
> > can't add that quite yet.)

Ah, after testing, we won't have to include the header lists at all, so you 
don't have to add that to a list. It turns out that if you pass the `include/` 
folder into the `include_directory` function, it automatically will monitor 
those files for changes. :) So we're good.

But, unless I'm mistaken, we'd still have to add that test to the 
`CMakeLists.txt`. Let me know if my thinking is wrong here.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100539
---


On Sept. 25, 2015, 5:46 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 5:46 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100539
---


Hey, it doesn't look like `src/tests/digest_tests.cpp` is being added to the 
`CMakeLists.txt` file in `3rdparty/libprocess/src/tests`. Is that correct? If 
not, could we please add it there as well?

(I have forgotten to add a list of header files as dependencies, so you can't 
add that quite yet.)

- Alex Clemmer


On Sept. 25, 2015, 5:46 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 5:46 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 25, 2015, 5:46 a.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

build fix


Repository: mesos


Description
---

Added SHA256 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

(Updated Sept. 25, 2015, 5:43 a.m.)


Review request for mesos, Gilbert Song and Timothy Chen.


Changes
---

minor fixes.


Repository: mesos


Description
---

Added SHA256 implementation.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38747: Adding digest utilities

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review100530
---


Bad patch!

Reviews applied: [38443, 38579, 38580, 38747]

Failed command: make -j3 distcheck

Error:
 make  dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d "mesos-0.25.0"; then find "mesos-0.25.0" -type d ! -perm -200 -exec 
chmod u+w {} ';' && rm -rf "mesos-0.25.0" || { sleep 5 && rm -rf 
"mesos-0.25.0"; }; else :; fi
test -d "mesos-0.25.0" || mkdir "mesos-0.25.0"
 (cd 3rdparty && make  top_distdir=../mesos-0.25.0 
distdir=../mesos-0.25.0/3rdparty \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
 (cd libprocess && make  top_distdir=../../mesos-0.25.0 
distdir=../../mesos-0.25.0/3rdparty/libprocess \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[3]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[3]: *** No rule to make target `src/tests/digest_tests.cpp', needed by 
`distdir'.  Stop.
make[3]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: *** [distdir] Error 1
make[2]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
make[1]: *** [distdir] Error 1
make[1]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
make: *** [dist] Error 2

- Mesos ReviewBot


On Sept. 25, 2015, 3:07 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> ---
> 
> (Updated Sept. 25, 2015, 3:07 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added SHA256 implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38747/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 38747: Adding digest utilities

[Jojy Varghese]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/
---

Review request for mesos, Gilbert Song and Timothy Chen.


Repository: mesos


Description
---

Added SHA256 implementation.


Diffs
-

  3rdparty/libprocess/Makefile.am 064310c61d8a0c5c5a87f305f1f62149dd3a6699 
  3rdparty/libprocess/include/Makefile.am 
fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
  3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38747/diff/


Testing
---

make check.


Thanks,

Jojy Varghese