Added errno generation in VMCI failures where errno was not set #1049
Conversation
There was a problem hiding this comment.
If I understand correctly, this change breaks retry logic in esx_vmdkcmd.go::run
This change guarantees that errno will be set in case of errors. This results in esx_vmdkcmd.go::run retrying "entire" operation/command. What happens if command is partially successful?
esx_service/vmci/connection_types.h
Outdated
| @@ -29,6 +29,13 @@ | |||
| #define START_NON_PRIVILEGED_PORT 1024 | |||
|
|
|||
| /* | |||
| * Check and set errno helper | |||
| * Useful when send/recv gets us less than we wanted, abd we want to set errno | |||
There was a problem hiding this comment.
@pdhamdhere - This PR simply bring it back to the original retry login and intent.
There is a short-term guard in place (put in a few weeks ago) to catch the case
else {
msg = fmt.Sprintf("Internal issue: ret != 0 but errno is not set. \
Cancelling operation - %s ", C.GoString(&ans.errBuf[0]))
}but the current one gets us back to the the original retry logic - which is to ignore the current exchange, reopen the socket and reissue command. This PR does not change any of that.
There was a problem hiding this comment.
Ok. For e.g. if create is partially successful i.e. ESX has created a disk but if we retry CREATE, does ESX returns success?
There was a problem hiding this comment.
yes, esx_service returns success on create() request for already existing disk .
esx_service/vmci/connection_types.h
Outdated
| * Useful when send/recv gets us less than we wanted, abd we want to set errno | ||
| * for the caller to know about the protocol Issue | ||
| */ | ||
| #define CHECK_ERRNO(_ret) {if (_ret >=0 && errno == 0) { errno = EBADMSG; }} |
There was a problem hiding this comment.
nit: _ret >= 0: space between "=" & "0"
There was a problem hiding this comment.
Sharp eye :-) !
Fixed
| @@ -154,6 +154,7 @@ vmci_get_one_op(const int s, // socket to listen on | |||
| ret, strerror(errno), b, MAGIC); | |||
| close(client_socket); | |||
| errno = saved_errno; | |||
| CHECK_ERRNO(ret); | |||
There was a problem hiding this comment.
Redundant since errno is set on earlier line. If it is for sake of consistency then you want to add CHECK_ERRORNO(ret) at other places in file too for e.g. L 169, L176
There was a problem hiding this comment.
it's not redundant since prior line recovers erno to the one before "close". If returned value was not MAGIC, errno "before close" would have been 0 and we need to set it.
We have some cases when protocol failed (e.g. EOF from client, or whatever). THis is indicated with 0 lenght returned , but errno was no set because it's considered success. This makes it inconsistent with CGO (and maybe others) who rely on errno to form "err" part of the return value
msterin
force-pushed
the
runci/errno.msterin
branch
from
502682c to
d696658
Compare
|
In all the cases where CHECK_ERRNO is invoked CONN_FAILURE (-1) is returned and errno is set. esx_vmdkcmd.go:Run() has code to handle when errno is not set and log an internal error. That should be removed as that condition can't happen any more. |
|
That's what internal error is - something that should never happen :-) |
We have some cases when protocol failed (e.g. EOF from client, or whatever).
This is indicated to with 0 length returned by recv(), but errno was nor\t set because EOF is considered a success.
This makes it inconsistent with CGO (and maybe others) who rely on errno to form "err" part of the return value, so this PR forces non-zero errno on all errors in protocol.
test: visual review and CI only