Re: [PATCH] diffie-hellman-group-exchange-sha256

[Yanis Kurganov <yanis.kurganov@xxxxxxxxx>]

2015-01-20 14:19 GMT+03:00 Andreas Schneider <asn@xxxxxxxxxxxxxx>:

>
> > Server side is ready!
>
> That's great. Thanks for your contribution! I would like to see some
> changes
> to the code for inclusion. It following our CodingStyle, small cleanups and
> splitting it into more patches.
>
> > My tests with dh-group-exchange client (PuTTY) are OK.
> > What about tests? What format of the tests do you want?
>
> Please do not call functions in if-statement or in function parameters.
> They
> make it harder to debug code.
>
> if (buffer_get_u32(session->in_buffer, &pbits) != sizeof(uint32_t)) {
>         ...
> }
>
> should be:
>
> int rc;
>
> rc = buffer_get_u32(session->in_buffer, &pbits);
> if (rc != sizeof(uint32_t)) {
>         ...
> }
>
> It makes it easier to debug code, also please also add braces to all
> if/for/while/etc. clauses!
>
> See also:
> http://git.libssh.org/projects/libssh.git/tree/README.CodingStyle
> and
> https://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
>
>
OK, done.
Also I made all-in-one patchset (with client and server side).


>
> I would also split the commit. Renames like dh_import_p ->
> dh_import_p_string
> should be in an own commit. They are not really related to diffie-hellman-
> group-exchange-sha256.
>
>
> dh_send_group_server() should use ssh_buffer_pack().
>
>
>
I have own libssh repository based on version 0.6.3 with other changes.
It's hard to split commits =(((
And I couldn't find ssh_buffer_pack. I suppose it is from master?


>
> Thanks!
>
>
>         -- andreas
>
>
>
>
>
>
> --
> Andreas Schneider                   GPG-ID: CC014E3D
> www.cryptomilk.org                asn@xxxxxxxxxxxxxx
>
>
>

Attachment: patchset.gz
Description: GNU Zip compressed data