-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NetICMP_TxEchoReq() Returns Wrong Error Code #3
Comments
Hi SeanAlling, |
In the case where If I understand your intent, then even when An alternative to what I proposed could be the following if(*p_err == KAL_ERR_NONE) {
NetICMP_LockAcquire(p_err);
KAL_SemDel(sem_handle, &kal_err);
if (*p_err == NET_ICMP_ERR_NONE) {
if (data_len > 0) {
if (p_echo_req_handle_new->DataCmp != DEF_OK) {
*p_err = NET_ICMP_ERR_ECHO_REPLY_DATA_CMP_FAIL;
}
}
}
if ((NetICMP_DataPtr->EchoReqHandleListStartPtr == p_echo_req_handle_new) &&
(NetICMP_DataPtr->EchoReqHandleListEndPtr == p_echo_req_handle_new)) {
NetICMP_DataPtr->EchoReqHandleListStartPtr = DEF_NULL;
NetICMP_DataPtr->EchoReqHandleListEndPtr = DEF_NULL;
} else if (NetICMP_DataPtr->EchoReqHandleListStartPtr == p_echo_req_handle_new) {
NetICMP_DataPtr->EchoReqHandleListStartPtr = p_echo_req_handle_new->NextPtr;
} else if (NetICMP_DataPtr->EchoReqHandleListEndPtr == p_echo_req_handle_new) {
NetICMP_DataPtr->EchoReqHandleListEndPtr = p_echo_req_handle_new->PrevPtr;
} else {
p_echo_req_handle_end = p_echo_req_handle_new->PrevPtr;
p_echo_req_handle_end->NextPtr = p_echo_req_handle_new->NextPtr;
}
p_echo_req_handle_new->NextPtr = DEF_NULL;
p_echo_req_handle_new->PrevPtr = DEF_NULL;
}
exit_kal_fault:
Mem_DynPoolBlkFree(&NetICMP_DataPtr->EchoReqPool,
p_echo_req_handle_new,
&lib_err);
exit_release_lock:
if(*p_err == KAL_ERR_NONE) {
NetICMP_LockRelease();
} The above would make the entire release branch and the exit_release_lock branch of code not execute when The if around This alternative proposed method also wont need an additional NET_ERR variable created and also does not need any extra code written for making sure the correct error code is set in p_err. |
After some more testing of this today, I realized my last remark had a memory leak. KAL_SemDel() would not be called when an error occurred which would prevent the created semaphore from being deleted. The fix is a small change to the if statement. release:
if(p_err == NET_ICMP_ERR_NONE) {
NetICMP_LockAcquire(p_err);
KAL_SemDel(sem_handle, &kal_err);
if (*p_err == NET_ICMP_ERR_NONE) {
if (data_len > 0) {
if (p_echo_req_handle_new->DataCmp != DEF_OK) {
*p_err = NET_ICMP_ERR_ECHO_REPLY_DATA_CMP_FAIL;
}
}
}
if ((NetICMP_DataPtr->EchoReqHandleListStartPtr == p_echo_req_handle_new) &&
(NetICMP_DataPtr->EchoReqHandleListEndPtr == p_echo_req_handle_new)) {
NetICMP_DataPtr->EchoReqHandleListStartPtr = DEF_NULL;
NetICMP_DataPtr->EchoReqHandleListEndPtr = DEF_NULL;
} else if (NetICMP_DataPtr->EchoReqHandleListStartPtr == p_echo_req_handle_new) {
NetICMP_DataPtr->EchoReqHandleListStartPtr = p_echo_req_handle_new->NextPtr;
} else if (NetICMP_DataPtr->EchoReqHandleListEndPtr == p_echo_req_handle_new) {
NetICMP_DataPtr->EchoReqHandleListEndPtr = p_echo_req_handle_new->PrevPtr;
} else {
p_echo_req_handle_end = p_echo_req_handle_new->PrevPtr;
p_echo_req_handle_end->NextPtr = p_echo_req_handle_new->NextPtr;
}
p_echo_req_handle_new->NextPtr = DEF_NULL;
p_echo_req_handle_new->PrevPtr = DEF_NULL;
}
else {
KAL_SemDel(sem_handle, &kal_err);
} Same as my prior recommendation, the block of code between the |
System Configuration
The following is my configuration.
The terminal is accessed through a serial port which is hosted over the CDC-ACM port. The shell at startup registered all net commands.
Bug Overview
NetICMP_TxEchoReq() function can return the wrong error code. In the following code, if
kal_err != KAL_ERR_NONE
then an error has occurred. For any error after settingp_err
the software will issue a jump torelease
.Once execution resumes at
release
, the first function,NetICMP_LockAcquire(p_err)
, will overwritep_err
resulting in any previous error code being lost.Reproduction
NET_ICMP_ERR_ECHO_REPLY_DATA_CMP_FAIL
).Shell should return
NET_ICMP_ERR_ECHO_REQ_TIMEOUT
.Debugging shows that
p_err
does get set correctly toNET_ICMP_ERR_ECHO_REQ_TIMEOUT
, but is overwritten once code followingrelease
starts executing.Resolution
If the switch statement is changes so that errors use
goto exit_kal_fault
instead ofgoto release
, the system will not callMem_DynPoolBlkFree()
which results in a memory leak. Since this code uses goto, the following code sectioncould be replaced with
where
exit_kal_fault_with_cleanup
will be used in the switch case instead ofgoto release
for the fault conditions.As I can't find a coding standard I do not know what the appropriate solution would be.
The text was updated successfully, but these errors were encountered: