Get rid of warnings in 64-bit

[Zenju]

Hi,

it would be great if I could compile libssh2 without warnings on 64-bit:

1. Currently I see lots of:
   warning C4267: 'argument' : conversion from 'size_t' to 'unsigned int', possible loss of data

Which is caused by the macros using "strlen" returning size_t but the _ex functions taking "unsigned int", e.g.

define libssh2_sftp_rmdir(sftp, path) \

libssh2_sftp_rmdir_ex((sftp), (path), strlen(path))

libssh2_sftp_rmdir_ex(LIBSSH2_SFTP *sftp, const char *path,
unsigned int path_len)

In general it seems that the "unsigned int" should be replaced by size_t since this is also what std::string::length() returns.

1. Here's another warning that's more serious, although we still have 23 years to go:
   warning C4244: '=' : conversion from 'int64_t' to 'unsigned long', possible loss of data

   std::int64_t mTime = ...;
   LIBSSH2_SFTP_ATTRIBUTES attribNew = {};
   attribNew.mtime = mTime; //32-bit target! loss of data!
   

2. There are more warnings in the implementation files that are shown when compiling libssh2 with Visual Studio 2013 in 64 bit.

[bagder]

I agree, no warnings would be ideal. But remember that we cannot change any types in the public API/ABI so where unsigned int is used today we cannot freely change it to size_t.

In the (1) case we need a typecast in that macro. The (2) thing is part of the protocol, isn't it?

Feel free to submit pull requests to fix warnings.

[Zenju]

remember that we cannot change any types in the public API/ABI so where unsigned int is used today we cannot freely change it to size_t.

I'm primarily interested in a practical solution to my current problem, which is that I see lots of compiler warnings, all of which are extremely useful to find 64-bit bugs in general. So I don't want to disable them, not even for a single translation unit.

On the other hand this means I cannot use the libssh2 function macros since they are not type-correct. One mitigation might be to add type casts to the macros.

This leaves the problem to the _ex functions only, but at least I can then type-cast on my own code, which is still wordy and ugly since I would prefer an API without the "len" parameters, which seem about CPU-perf optimization only.

The (2) thing is part of the protocol, isn't it?

Yes, maybe it's time to upgrade to a newer version of the spec?
https://filezilla-project.org/specs/draft-ietf-secsh-filexfer-13.txt

Just to clarify, I'm no expert in this area at all, basically I know SFTP and libssh2 for a week now :)

Feel free to submit pull requests to fix warnings.

I'm a C++ programmer, so I would probably fail to get the C style right. Still I created a patch adding the C-casts, but chances are it won't work since I use code-formatting in my projects.
I'l try to attach it...

[bagder]

Sure we could implement support for a newer SFTP version. I don't think anyone will object to that. But that is a rather big undertaking that I'm not personally prepared to do anytime soon. We'll welcome help and patches of course!

[eschaton]

Would it be reasonable to define a new type, say libssh2_size_t, and use that instead? Then it could be a configuration option whether to stay ABI-compatible with the deployed library (via typedef unsigned int libssh2_size_t) or to use the system size_t.

The latter would be useful to those embedding libssh2 who don't have to worry about ABI-compatibility, and it would also allow building it with the system size_t locally just to run tests.

[bagder]

Seems like a reasonable approach to me!

[eschaton]

I wrote that up as a separate issue (#29) since it wouldn't actually address these compiler warnings in many cases, but (I think) it would be a good general cleanup.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.