at least partial support to scp >2GB files on windows

[dbyron0]

I have a sneaking suspicion there's more to it than this (even beyond adding the equivalent check to autoconf...if people want that), but it's a start. This is the warning that makes me nervous:

1>..\..\src\scp.c(731) : warning C4244: '=' : conversion from '__int64' to '_off_t', possible loss of data

so perhaps there's _stat64, etc. work to do as well. I'm guessing because I don't have a great way to test this beyond the testsuite at the moment.

[dbyron0]

See #31

[bagder]

Is the warning shown perhaps because off_t is unsigned? Otherwise it certainly looks like a patch worth testing!

[dbyron0]

C:\Program Files\Microsoft Visual Studio 9.0\VC\crt\src\sys\types.h on my machine has off_t typedef'd to a long (i.e. a signed type).

[dbyron0]

There's also this: https://msdn.microsoft.com/en-us/library/323b6b3k.aspx

[bagder]

Ouch long is 32 bit on Windows... we need to fix that too then.

[bagder]

In the curl project we have a this code block: https://github.com/bagder/curl/blob/master/lib/curl_setup.h#L355

See the struct_stat hack in particular...

[dbyron0]

Yup, that looks useful. This appears to be an interface change, at least for windows. Is there anything special to do to make that (more) OK?

[dbyron0]

Sorry, here's the possible interface change I see: https://github.com/libssh2/libssh2/blob/master/include/libssh2.h#L810

[bagder]

Ack yes, we need to provide a new function using another struct. I never liked using the systems stat struct like that and now it hit us in the head...

I would argue that we should use a private stat clone which we know how it looks.

[dbyron0]

Any chance you've got a >2GB file sitting on a server I can hit for testing?

[bagder]

Yes I do, just set one up! I'd just not like to announce it here to the world to torment my machine. Can I give it to you via a less public means somehow?

[dbyron0]

Thanks much. Just sent you an email.

[dbyron0]

This is only used in an example (e.g. https://github.com/dbyron0/libssh2/blob/large_file_windows/example/scp_nonblock.c#L265) so perhaps it's better to duplicate some logic and move it there....

[dbyron0]

OK, I think this is worth a look. It doesn't feel particularly clean, but I think it gets the job done. Easy for me to say as I haven't tested beyond building and running tests using cmake on windows, and autotools on cygwin and os x.

[dbyron0]

I made a curl branch to test this here: https://github.com/dbyron0/curl/tree/large_file_windows

[keith-daigle]

Hi , I tripped over this bug just a few days ago but found that there's a little more than just the struct stat needed to fix it on the visual studio builds. It's pretty simple, the hooks for using long long are already there, just a couple of defines need to be added to the config.h. Once that was done and I redefined off_t to long long I got away with the system struct working correctly. This is obviously a UGLY hack but was enough to be a proof of concept.

My initial thought was to add a libssh2_scp_recv64 to match the other functions with a new signature, or use a #define to have scp_recv take one of the _stat64 structs on win32. If either of those are preferable I can code it up and test it on win32/64 against a few different compilers.

I forked dbyron's large file solution and added the changes I know need to go in the win32/libssh2_config.h header for builds on visual studio to enable the long long support in the build. I'm going to try and build tomorrow and see if it looks good with the rest of dbyron's solution as-is. If it's OK who should the pull request go to?

[dbyron0]

Thanks for picking up the ball here. I have a feeling the way forward is for me to rebase against the head of upstream/master and then to merge your changes here. Though it probably doesn't matter much if your fork is up to date and you create a pull request directly.

Either way, it's probably time to make a pull request in curl for the corresponding changes there.

Please let me know if my rebasing is going to cause you a struggle. If not, I'll go ahead with that.

[keith-daigle]

Go ahead and rebase it. It looks like the VS project just needs a few defines and it should be OK. I'll be able to test the native visual studio builds for sure but I'll have to look into cmake.

UPDATE: I just took a closer look at the current state of master and it looks like at commit af14462 there were changes made that do most of what I needed to do. So once the rebase is done it should be a breeze.

[dbyron0]

Great, rebase done.

[dbyron0]

OK, I think I've taken care of the split in dbyron0@e803839. I'm happy to change things to caps, but I figured I'd take baby steps.

[keith-daigle]

I just rebased and sent a pull request to you dbyron0. I tested today with dbyron0/libssh2@2f67c8d and found that I was able to build successfully and pull 10+ GB files with 32 bit builds. I tested both with vs2005 and vs2010. I see that you've got other cmake items to sort out, but I don't think it matters any to the few minor tweaks I made.

My commit does do one thing and that's to put the .dsp file back into the win32 directory since it's needed by anyone who wants to use visual studio. I noted where it was removed so maybe someone can see if Daniel really wants it removed for good for reasons that weren't in the commit msg.

[bagder]

@keith-daigle are you referring to the commit a4fdf0d ?

In general I think we should push harder towards using cmake for building, especially on windows.

[dbyron0]

I'm repeating the question I asked here: https://github.com/dbyron0/libssh2/pull/1/files#r38787726, but perhaps for a wider audience. I guess I'm also agreeing with @bagder above, though lingering slightly off topic. Do we still need the win32 directory? Does cmake generate everything we need? It looks like https://github.com/libssh2/libssh2/blob/master/src/CMakeLists.txt#L190 refers to https://github.com/libssh2/libssh2/blob/master/win32/libssh2.rc, so I guess we still need bits of it.

[keith-daigle]

@Badger yes, that's what I'm referring to. And I agree with @dbyron0 that the win32 directory is confusing since upon first glance at the repo it's where I immediately went to start building. I wasn't really thinking that I should be using cmake to generate the VS projects and I was using that old win32 project when I tested.

I'll test by using cmake and let you know how it works out. If it's all good is maybe replacing all the un-needed contents of the win32 directory with a readme about where the files went and the cmake usage acceptable?

[alamaison]

When I added CMake support, I didn't remove the win32 directory because people still use it. I believe @gknauf is still actively maintaining the old project files.

Guenter, maybe you can clarify, do you still use the project files in /win32? If so, what advantages do they have over generating them using CMake?

[dbyron0]

OK, @keith-daigle's changes are in. I think this is ready to go...unless @alamaison has some more comments.

[alamaison]

I'll review at the weekend (on the move right now). Have the #defines been
capitalised and renamed to avoid undeffing the standard functions?

On Wed, 9 Sep 2015 23:38 David Byron notifications@github.com wrote:

OK, @keith-daigle https://github.com/keith-daigle's changes are in. I
think this is ready to go...unless @alamaison
https://github.com/alamaison has some more comments.

—
Reply to this email directly or view it on GitHub
#31 (comment).

[dbyron0]

They haven't. With any luck I'll get to it before you review again.

[dbyron0]

After a closer look, it appears I dragged too much from curl. I couldn't find anywhere in libssh2 that uses lseek or fstat, so those were pretty easy to convince myself to remove. The only instances of stat I could find are https://github.com/libssh2/libssh2/blob/master/example/scp_write.c#L93 and https://github.com/libssh2/libssh2/blob/master/example/scp_write_nonblock.c#L132 which don't include https://github.com/libssh2/libssh2/blob/master/src/libssh2_priv.h.

So, I'm planning to squash these three 'extra' commits (dbyron0@28d96e4, dbyron0@d54771b and dbyron0@6749859) but I thought I'd reference them for posterity.

It does appear that the two examples need some tweaking to support large files, but I'm tempted to consider out of scope for this change. Thoughts?

[alamaison]

Sounds good to me.

Could you either capitalise libssh2_struct_stat and libssh2_struct_stat_size or typedef them instead? Preferably the latter.

[dbyron0]

I went for the typedefs, and rebased against upstream. Mind taking another look?

[alamaison]

Looks good to me :)
I'll merge this when I get a chance

[dbyron0]

Thanks much.

[alamaison]

Merged. Thanks

[dbyron0]

Thanks much. For those interested, changes to curl to use this are here: curl/curl#451.