Properly handle successful Run() with stale errno #999
Conversation
msterin
force-pushed
the
stale_errno.msterin
branch
from
15c0db3
to
c861ba0
Compare
Sometimes vmci_server.c does not set errno when returning Success. In this cases stale errno (e.g. from priorly failed syscall) might be accidentally picked up. The commit fixes this issue, and also improves error printing for inotify failures. Sometimes vmci_server.c does not set errno when returning Success. In this cases stale errno (e.g. from priorly failed syscall) might be accidentally picked up. The commit fixes this issues, and also improves error prinitng for inotify failures Cleaned up eror msg
| if errno == syscall.ECONNRESET || errno == syscall.ETIMEDOUT { | ||
| msg += " Cannot communicate with ESX, please refer to the FAQ https://github.com/vmware/docker-volume-vsphere/wiki#faq" | ||
| ret, err = C.Vmci_GetReply(C.int(EsxPort), cmdS, beS, ans) | ||
| if ret != 0 { |
There was a problem hiding this comment.
Are we doubly sure that ret == 0 && err != nil will never happen?
There was a problem hiding this comment.
Oops. Re-read the PR description. LGTM.
There was a problem hiding this comment.
it happens all the time on alpine ::-), it simply means the errno is not properly reset (by someone else) and we may see the stale one. I did see it when working with the managed plugin on Alpine distro. It is explicitly a non-event for us since return 0 means we are totally good. It kind of breaks Go purity (err is what matters there) in favor or C "err code" return in return valye, so probably a comment is due.
msterin
force-pushed
the
stale_errno.msterin
branch
from
c861ba0
to
68deb4d
Compare
|
Aha ! the CI picked up the new error message: @govint - that seems to confim the conclusion about "we return err, do not set errno, this fooling the code into improperly handle memory in vmci exchange with Go. I will go ahead and merge, we should expect more CI failures before you adjust and merge #941. What's the ETA on this one ? |
Sometimes vmci_server.c does not set errno when returning Success.
In this cases stale errno (e.g. from priorly failed syscall) might be
accidentally picked up. The commit fixes this issue, and also improves
error printing for inotify failures
@govint- it is a light modification of a fragment of your PR. I do not want to wait for the whole PR to come in but it is blocking me, so sending this piece as a separate PR
TEST: manual test on Alpine only (using it for managed plugin conversion). Before the test ALL commands in my runs (including "docker volume ls") where failing with "file not found" which was a leftover from something else. After the fix the issues disappeared.
//CC @govint @pdhamdhere