mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jojy Varghese" <j...@mesosphere.io>
Subject Re: Review Request 38747: Adding digest utilities
Date Tue, 13 Oct 2015 15:42:54 GMT


> On Oct. 13, 2015, 12:54 a.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, lines 85-120
> > <https://reviews.apache.org/r/38747/diff/14/?file=1092722#file1092722line85>
> >
> >     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<DigestImpl*> 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<Digest::SHA224>                                          
{                                                                                
  typedef SHA224_CTX ctx_type;                                                   
  static const size_t digest_length = SSHA224_DIGEST_LENGTH;                      
};

template <>                                                                      
DigestFunctionTraits<Digest::SHA224>::Init_fn                                    
  DigestFunctionTraits<Digest::SHA256>::Init = SHA224_Init;                      
                                                                                 
                                                                                 
template <>                                                                      
DigestFunctionTraits<Digest::SHA224>::Update_fn                                  
  DigestFunctionTraits<Digest::SHA256>::Update = SHA224_Update;                  
                                                                                 
                                                                                 
template <>                                                                      
DigestFunctionTraits<Digest::SHA224>::Final_fn                                   
  DigestFunctionTraits<Digest::SHA224>::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::SHA256>::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
> > <https://reviews.apache.org/r/38747/diff/14/?file=1092722#file1092722line159>
> >
> >     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::SHA256>::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:
> 
> -----------------------------------------------------------
> 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
> 
>


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