Subject: Re: Suggested patches for WinCNG

Re: Suggested patches for WinCNG

From: Marc Hoersken <info_at_marc-hoersken.de>
Date: Thu, 20 Mar 2014 07:00:56 +0100

Hi Bob,

thanks for your contribution. In addition to the comments by Daniel and
Peter, my comments are in line:

On 19.03.2014 22:37, Bob Kast wrote:
> Libssh2.h:
>
> In Windows, a socket is of type SOCKET, not int.
>

Please post this as a seperate patch as it is not WinCNG-specific.

> Libssh2_priv.h:
>
> Redundant #define inline
>

It might be there for a reason. Please do not forget that there are
other Windows-compilers except Visual Studio. For example, I use MinGW
and MinGW-w64 to compile libssh2. It might be worth to add an #ifndef
around the #define in that case.

> Openssl.c:
>
> You shouldn’t need Openssl to compile if you aren’t selecting it.
>

That makes perfect sense with Peter's recent changes to the backend
selection. We just need to make sure that LIBSSH2_OPENSSL is defined as
the default within all buildfiles in order to be backwards compatible.

> Wincng.c:
>
> Putting the #pragma for the libraries works for both DLL and LIB versions.
>

This is rather compiler-specific to Visual Studio than
platform-specific. The #ifdef WIN32 ist not sufficient in that case,
since MinGW gcc compilers won't understand the pragma statements. I
would rather suggest adding the libraries to the AdditionalDependencies
element of the Link section within your Visual Studio project files.

> _libssh2_wincng_hash_update: the parameter needs to be const to match.
> It is interesting that C just gives a warning for that.
>

But is casting from (const unsigned char *) to (unsigned char *)
actually a real fix for this or just hiding things?

> pPaddingInfo value is undefined. Doc says it must be NULL if not used.
>

Good catch, that's correct.

> Wincng.h:
>
> STATUS_SUCCESS unfortunately not defined if using non-driver includes.
>

I will have to take a look at this. It works for MinGW. Please give me
some time over the weekend to setup my own Visual Studio 2012 to build
libssh2 and verifiy your suggestions.

> _libssh2_wincng_hash_ctx and _libssh2_wincng_key_ctx: I found this
> confusing. Kind of a circular #define. I think this is more of what
> was intended.
>

This change is okay with me. I think I used defines, because it the
approach was used within other places of libssh2. Daniel, Peter, what do
you think?

> Forward declarations: without these the compiler complains for each call.
>

MinGW gcc does not complain and the other backends do not have them.
Again, please allow me some time to verify this.

> Libssh2_config.h:
>
> Pragma to suppress “possible loss of data” warnings.
>

A compiler-specific flag would could also be put into the Visual Studio
project configuration.

> New Files:
>
> -Libssh2.sln – solution file
>
> -Libssh2.vcxproj, libssh2.vcxproj.filters – project files for libssh2
>
> -Tests.vcxproj, tests.vcxproj.filters – project files for tests – I
> haven’t tested this.
>
> These are specifically for VS2013 and make it easy to create DLL/LIB,
> Debug/Release, 32/64 bit builds.
>

I think we should rather wait for Alexander Lamaison's CMake files since
they will allow us to create generic VS project files that are not tied
to a specific VS and SDK version. For example, I still use VS 2012 for
my own projects.

Thanks again.

Best regards,
Marc
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2014-03-20