Hotfix/fix dcs flow issues #807
Conversation
itayx
requested review from
arnout,
morantr,
rmelotte and
tomereli
as code owners
|
itayx
force-pushed
the
hotfix/fix-dcs-flow-issues
branch
from
7c40b1b to
3a82a48
Compare
ghost
added this to the 
| @@ -3359,7 +3359,8 @@ bool master_thread::handle_cmdu_control_message(const std::string &src_mac, | |||
| return false; | |||
| } | |||
|
|
|||
| auto radio_mac = notification->radio_mac(); | |||
| //get the mac from hostap_mac | |||
| auto radio_mac = network_utils::mac_from_string(hostap_mac); | |||
There was a problem hiding this comment.
This sounds wrong... "hostap" is normally used to indicate the VAP, not the radio.
But looking at the definition of hostap_mac, it is in fact wrongly named, because it takes the radio_mac from the beerocks header. So it's OK.
That said, a much better change would be to (in one commit) change the beginning of the function to
auto radio_mac = beerocks_header->actionhdr()->radio_mac();
auto hostap_mac = network_utils::mac_to_string(radio_mac);
and (optionally) in a second commit remove most instances of hostap_mac (in the logging, you can just use an sMacAddr directly now).
Also, the commit message should explain why you do this. The commit message just says "because we can". I thought it would be because you're going to remove the radio_mac from cACTION_CONTROL_CHANNEL_SCAN_TRIGGERED_NOTIFICATION, but you don't do that.
There was a problem hiding this comment.
Since this change would effect more then just the DCS feature,
I'm going to create a new issue for it (assigned to me), and I'll add a Hotfix to solve this once I'm finished with my current issues
| @@ -3359,7 +3359,8 @@ bool master_thread::handle_cmdu_control_message(const std::string &src_mac, | |||
| return false; | |||
| } | |||
|
|
|||
| auto radio_mac = notification->radio_mac(); | |||
| //get the mac from hostap_mac | |||
There was a problem hiding this comment.
This comment is useless. It basically says "get the mac from the mac".
| //get the mac from hostap_mac | ||
| auto radio_mac = network_utils::mac_from_string(hostap_mac); |
| //get the mac from hostap_mac | ||
| auto radio_mac = network_utils::mac_from_string(hostap_mac); |
| //get the mac from hostap_mac | ||
| auto radio_mac = network_utils::mac_from_string(hostap_mac); |
| auto msg = static_cast<bwl::sCHANNEL_SCAN_RESULTS_NOTIFICATION *>(data); | ||
|
|
||
| auto &in_result = msg.channel_scan_results; | ||
| auto &in_result = msg->channel_scan_results; |
There was a problem hiding this comment.
Although the code indeed looks better like this, it does nothing to fix the issue mentioned in the commit message. in_result is still the same reference as it was before, so it will become invalid as soon as the lifetime of data ends.
I also don't see any problem with the lifetime of in_result. It is only used within this event handler, and no references are taken anywhere.
| @@ -1216,7 +1216,8 @@ int cli_bml::set_dcs_continuous_scan_params_caller(int numOfArgs) | |||
|
|
|||
| std::string::size_type pos; | |||
| //[radio_mac=<radio_mac> dwell_time=<dwell_time> interval_time='<interval_time>']" | |||
| for (int i = 1; i < numOfArgs; i++) { //first optional arg | |||
| for (int i = 0; i < numOfArgs; i++) { //first optional arg | |||
| std::cout << "args[" << i << "]: " << args.stringArgs[i] << std::endl;; | |||
There was a problem hiding this comment.
Do we really want to leave this debug thing in here?
There was a problem hiding this comment.
No.
It was an oversight that it stayed, removed due to it being unneeded
|
Another little nitpick: @itayx you often use "crush" instead of "crash". Also some commit messages have wrong capitalisation or weird wrapping. |
When ACTION_MONITOR_CHANNEL_SCAN_RESULTS_NOTIFICATION is received from the monitor, an ACTION_CONTROL_CHANNEL_SCAN_RESULTS_NOTIFICATION should be sent to the controller. Change from ACTION_MONITOR_CHANNEL_SCAN_RESULTS_NOTIFICATION to ACTION_CONTROL_CHANNEL_SCAN_RESULTS_NOTIFICATION. Signed-off-by: Adam Dov <[email protected]>
the hostap_mac is provided by the agent as part of the CMDU msg. get radio_mac (sMac) from hostap_mac (string) using the wireless_utils API Signed-off-by: Adam Dov <[email protected]>
The sCHANNEL_SCAN_RESULTS_NOTIFICATION message was dereferenced directly to channel_scan_results param, but then again dereferenced. This caused a crash when freeing the buffer at the end of the scope. Change, so the first dereference is done to the buffer and only the second dereference is to the param inside the msg. Signed-off-by: Adam Dov <[email protected]>
The bml cli command used to set the continuous params has an issue where it ignores index 0 of the given arguments, which depending on the arguments being sent could cause the next function in the flow to crush Changed the initial index to 0 Signed-off-by: Itay Elenzweig <[email protected]>
itayx
force-pushed
the
hotfix/fix-dcs-flow-issues
branch
from
3a82a48 to
3182be6
Compare
all DCS code has been ported #562
DCS Validation has been started on both RDKB/UGW platforms.
This PR is used to fix small issues throughout the codebase in the DCS feature's flow