Adding pcap mock test. finding and fixing bugs using it. #1304
Conversation
Added a libpcap mock for testing the BMI code in P4 behavioral-model by injecting packets using the actual BMI code instead of their nanomsg based injector (which is bypassing the BMI code) It is not meant to be a full mock of libpcap, it only implements the functions required for this special purpose. Bugs fixed bmi_port.c: In bmi_port_destroy_mgr function, remove interfaces in reverse order. The previous code removed only half of the interfaces because it was iterating through an array while removing its members. In _bmi_port_interface_remove function, bug fix to prevent a deadlock. Function uses "write" to write on a pipe and waits for the answer using "read" while it holds the lock "port_mgr->lock". Meanwhile, the "run_select" thread requires to get that lock once per iteration of the loop, causing a deadlock. dev_mgr_bmi.cpp: In BmiDevMgrImp class, p_monitor should be stopped in destructor and before destruction of port_mgr to prevent pure virtual function call.
antoninbas
left a comment
antoninbas
left a comment
There was a problem hiding this comment.
biggest concern is that I am not convinced about the correctness of the change in _bmi_port_interface_remove (but I agree that the current code suffers from a deadlock).
| pthread_rwlock_unlock(&port_mgr->lock); | ||
| read(port_mgr->socketpairfd[0], buf, sizeof(buf)); | ||
| pthread_rwlock_wrlock(&port_mgr->lock); |
There was a problem hiding this comment.
I agree there is a possible deadlock and it needs to be fixed, however I don't see any justification that the change is correct, and no explanation as to the placement of the unlock instruction (why not right after the FD_CLR call?).
For example, any possible issue if there is a concurrent call to bmi_port_interface_add with the same port_num?
| @@ -0,0 +1,10 @@ | |||
| AM_CXXFLAGS = -g -pthread -std=c++20 | |||
There was a problem hiding this comment.
The codebase has not been updated to C++20 yet, so please stick to C++17, or submit a separate change to update the code base to C++20
There was a problem hiding this comment.
why not support make test_packet_redirect.cpp parametrized so it can run in both "dataplane" modes? This is supported by the google test library. It would avoid a lot of duplication as far as I can tell.
There was a problem hiding this comment.
The rest of the codebase uses the .h extension for headers unless my memory is failing me
|
|
||
| std::string name_; | ||
|
|
||
| bool is_activated_ = false; |
There was a problem hiding this comment.
maybe add a comment to explain this field
| return no_error; | ||
| } | ||
|
|
||
| int sendpacket(const u_char* buf, int size) { |
| targets/l2_switch/learn_client/Makefile | ||
| targets/simple_switch/Makefile | ||
| targets/simple_switch/tests/Makefile | ||
| targets/simple_switch/tests/pcap_mock/Makefile |
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment, or this will be closed in 180 days |
2 participants
Added a libpcap mock for testing the BMI code in P4 behavioral-model by injecting packets using the actual BMI code instead of their nanomsg based injector (which is bypassing the BMI code).
It is not meant to be a full mock of libpcap, it only implements the functions required for this special purpose.
This test was used to find and fix 3 bugs.
Bugs fixed:
bmi_port.c:
In bmi_port_destroy_mgr function:
The previous code removed only half of the interfaces because it was iterating through an array while removing its members.
In _bmi_port_interface_remove function:
Bug fix to prevent a deadlock. Function uses "write" to write on a pipe and waits for the answer using "read" while it holds the lock "port_mgr->lock". Meanwhile, the "run_select" thread requires to get that lock once per iteration of the loop, causing a deadlock.
dev_mgr_bmi.cpp:
In BmiDevMgrImp class:
p_monitor should be stopped in destructor and before destruction of port_mgr to prevent pure virtual function call.