On Monday 12 January 2015 16:29:12 Will Cosgrove wrote:
> Hi All,
> I’m adding diffie-hellman-group-exchange-sha256 support and have it working.
> However, if I am to submit this patch back to the project I have a couple
> code style questions.
>
> First, kmdhgGPsha1kex_state_t is coded to be specific to sha1. No big deal
> I thought, I could add a sha256 version. However that leads to
> key_exchange_state_low_t which is included in key_exchange_state_t. So now
> we’re duplicating three structs and causing a lot of branching, not so
> great.
>
> At that point, I decided to change kmdhgGPsha1kex_state_t to support sha256.
> The following changes were made:
>
> unsigned char h_sig_comp[SHA256_DIGEST_LENGTH]; //SHA1_DIGEST_LENGTH
>
> //libssh2_sha1_ctx exchange_hash;
> EVP_MD_CTX exchange_hash;
>
> This isn’t so hot as it hard-codes openssl support instead of using the
> libssh2_sha1_ctx macro. On the flip side, creating three new structures
> for a couple calls seems excessive.
>
> Anyone out there have opinions on how to proceed?
For me it is difficult to infer what exactly you are proposing to change
from the above description. Could you please attach the current version
of your patch to clarify it?
In general, avoiding duplicated code is good. On the other hand, I believe
that the only module where OpenSSL should be hard-coded is src/openssl.c
whereas the structures defined in libssh2_priv.h should be independent on
a particular crypto library.
Kamil
> Cheers,
> Will
> _______________________________________________
> libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2015-01-13