search

tjibbevanderlaan / chromeos-filesystem-sftp

ChromeOS app to access SFTP server
https://chrome.google.com/webstore/detail/shared-network-folder-sft/gbheifiifcfekkamhepkeogobihicgmn
BSD 3-Clause "New" or "Revised" License
80 stars 21 forks source link

Converting from NaCl to PNaCl #58

Closed fedoracooper closed 9 years ago

fedoracooper commented 9 years ago

PNaCl is strongly recommended over NaCl for app compilation: https://developer.chrome.com/native-client/nacl-and-pnacl

I have been able to compile via the pnacl toolchain instead of newlib with minimal changes to the Makefile, mainly disabling a compiler warning about unused private fields. This also requires a couple other modifications to point at the new path.

The zip file becomes much smaller (2.8MB instead of 15MB, and we could probably make it even smaller by removing the _unstripped.pexe file), and it seems to run just as well, if not faster than with the separate nexe files per architecture. The only change I noticed was that first-time run took a few seconds to show the fingerprint confirmation window, which I assume is because of dynamic compilation on the machine.

Also, I would recommend not adding the new pnacl binaries folder to git, as you typically don't want to check in binaries in source control. This makes the repo smaller, and people can recompile at anytime anyway.

fedoracooper commented 9 years ago

This is not yet ready for prime time. Upon further testing, while file downloads appear to work fine, file uploads seem to cause the NaCl module to crash almost immediately. I'm not sure what the problem is at this point.

yoichiro commented 9 years ago

@fedoracooper Thank you for big contribution! Yes, I tried to use PNaCl, however, I have same experience that both downloading/uploading failed...

yoichiro commented 9 years ago

@fedoracooper BTW, the reason that I committed the compiled files is that I can't build the NaCl module on Chromebook. I already spend my developing on my Chromebook, and writing time JavaScript code was longer than writing C++ code recently to me. But, yes, you are right. We should add the compiled files of C++ code, I think, too.

mtomasz-chromium commented 9 years ago

If PNaCl build fails, while NaCL works, then most probably there is a bug in the SFTP extension code. Maybe some memory corruption?

mtomasz-chromium commented 9 years ago

So, I'd strongly suggest to use gdb and track that crash with PNaCL, as it may be an obvious bug, which should be fixed. I was getting random crashes before on SFTP NaCL build, so it may be the same root cause.

fedoracooper commented 9 years ago

I spent the time figuring out how to debug w/ gdb on my chromebook via SSH to my linux box. The problem is when AbstractCommand::OpenFile is called. libssh2_sftp_open is returning -31 (LIBSSH2_ERROR_SFTP_PROTOCOL), and when I call libssh2_sftp_last_error again, it returns 2 (SSH_DISCONNECT_PROTOCOL_ERROR). This causes the code to throw an exception which triggers a SIGILL to terminate the program.

So something strange is happening in libssh2_sftp_open with the LIBSSH2_FXF_WRITE option and the pnacl libraries. I am using Pepper 41. I don't understand how this could be a libssh2 issue, because I'm using the same Pepper version for newlib and pnacl. I wonder if PNaCl is somehow different with respect to network handling?