winnfsd / vagrant-winnfsd

Manage and adds support for NFS for Vagrant on Windows.
Other
570 stars 61 forks source link

Improve error handling upon startup failure #110

Closed cbj4074 closed 2 years ago

cbj4074 commented 7 years ago

When winnfsd.exe fails to start for any reason, nfsservice.bat echoes started, and steams on ahead, with no indication that a failure occurred.

For example, if the port bindings cannot be made, because the ports are already in use, winnfsd.exe will fail to start, and Vagrant will complain only once the mount ... command fails over SSH.

This slight modification at least gives the user a hint as to what may be wrong.

Given that winnfsd.exe does not return a non-zero exit code if it fails to start, we have little choice but to check to see if it is running after we attempt to start it. (This logic would be a lot simpler if the winnfsd.exe could be made to return proper exit codes on failure.)

It is necessary to pause/sleep briefly after the attempt to start winnfsd.exe, because it may take a second or two for the program to exit on failure. While a bit "hacky", the ping 127.0.0.1 -n 3 > nul command works well and is available on all relevant versions of Windows. (The timeout command is absent from Windows XP, which while end-of-life, makes it a less favorable choice.)

Finally, I've prefixed the various executable paths with %windir%\system32, which eliminates potential conflicts with Cygwin or Git executables, for example (find, in particular), as many Vagrant users have one or both installed.

As I said, it would be far more ideal to modify winnfsd.exe to issue proper return codes in various failures states, but this is an improvement that can be made in the meantime.

Thanks for considering!

P.S. I can't seem to eliminate that newline change at the end of the file; GitHub seems to be doing that on its own.

QWp6t commented 7 years ago

if you want to use timeout, you can use timeout and fallback to ping if timeout isn't available.

timeout & if errorlevel 1 ping 127.0.0.1
cbj4074 commented 7 years ago

Even better! Thanks, @QWp6t .

I will do some testing and update the PR once I've confirmed that your suggestion works as intended.

cbj4074 commented 6 years ago

@QWp6t How's that look now?

I've tested it in all scenarios (timeout.exe available and not) and the script seems to behave as expected.