-
Notifications
You must be signed in to change notification settings - Fork 14
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
Connman upgrade to 1.33 #66
Merged
LaakkonenJussi
merged 15 commits into
sailfishos:master
from
LaakkonenJussi:connman_upgrade_to_1.33
May 20, 2024
Merged
Connman upgrade to 1.33 #66
LaakkonenJussi
merged 15 commits into
sailfishos:master
from
LaakkonenJussi:connman_upgrade_to_1.33
May 20, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spiiroin
requested changes
Apr 30, 2024
LaakkonenJussi
force-pushed
the
connman_upgrade_to_1.33
branch
3 times, most recently
from
May 15, 2024 13:35
b54b09f
to
f0e555f
Compare
spiiroin
reviewed
May 16, 2024
LaakkonenJussi
force-pushed
the
connman_upgrade_to_1.33
branch
from
May 17, 2024 11:13
dae3c36
to
c155a68
Compare
spiiroin
approved these changes
May 20, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All my concerns have been addressed
This patch contains new implementations for following: 1. Band Steering and Load balancing Support: Enterprise AP sometimes denies association with assoc status code 17. The reason AP sends this code, is to steer to some other AP (in a different band) or for doing load balancing (in case this AP is heavily loaded). This will not work with current connman as connman would take this disconnect notification as normal disconnect and disable the network. This act would cause wpa_s search for the next BSSID stop. The next time connect attempt is issued from connman, the story would repeat again. The idea of this patch is to not interfere with BSSID selection logic of wpa_s. wpa_s was sending disconnect reason code on DBUS as a part of PropertyChanged signal but there was no notification for assoc status code. In this commit id of hostapd (c7fb678f3109e62af1ef39be9b12bf8370c35bde) wpa_s is also sending assoc status code as a part of DBUS PropertyChanged signal. Idea here is that on a disconnect notification from wpa_s connman would look into the assoc status code and if it is set to 17, it would not proceed. This will let wpa_s continue with its own BSSID selection logic. 2. Optimize DBUS call - In case network path is Not NULL (means a profile is already plumbed in wpa_s), network_remove should only be called if the new network is different from what is sitting in wpa_s. The notion of new network here is SSID, security type and passphrase. If the new network is same as the one in wpa_s, no need to do remove, add and select. This would save 3 DBUS calls to wpa_s. 3. Fast Reconnect: On receiving a deauth with reason code 6, 7 or 4 wpa_supplicant will immediately try to connect back. Now with current implementation of connman, a disconnect notification will cause network to get disabled and connection procedure would stop. This also impacts the roaming time in case disconnect is because of any other reason code. The act of disabling network severly affects wpa_s connection state machine as it would generate a deauth to current network when half way the connection was done. The idea here is that we do not disable network on disconnect and let wpa_s keep trying to find out that network. Only when connman has another network this network would be removed and new network would be added. (cherry picked from commit 6245582)
Adding connectable flag in the network structure and API to set and get that flag. The set API can be used to mark a particular network connectable so that this network is never removed or maked unavailable. (cherry picked from commit b7d20de)
On network_connect, mark previosuly connected network's connectable flag as false and current network's one as true. Also do not remove the connected network on network_removed. (cherry picked from commit ff9decb)
Do not mark a network unavailable if connectable flag is true. Similarly do not remove a network if connectable flag is set. (cherry picked from commit 9aad337)
Currently the property Connected is not used for P2P Technology thus it always remains False. This patch aims to keep it updated in order to make easier for the UI to show if P2P technology has an active P2P connection or not. This implementation has a limitation in cases of complex systems where multiple P2P interfaces are present and can be connected simultaneously, it is because the disconnection of one of those interfaces will cause that the property becomes False even if there is another interface still connected. But let say that such a complex systems are not very common. (cherry picked from commit b0bb2e4)
g_strdup() will return NULL only when ifname is NULL. This patch checks ifname before any memory allocation and free is done. (cherry picked from commit a8a2ad2)
If the wpa_supplicant has been launched before ConnMan, when the Connman calls the GetAll method on fi.w1.wpa_supplicant1, the properties are returned as an array where there is an entry named "Interfaces". This entry is an array itself with only the object path of the current available interfaces. The problem occurs when ConnMan goes across this array using the function interface_added(), because this function tries also to extract the interface properties next to object path which is wrong because properties are not appended in this message reply. So, by doing dbus_message_iter_next(iter), we are actually skipping one interface of the list and it will result in a future error because the skipped interface(s) were not stored and ConnMan will not be aware of them. This issue is due to the function interface_added() is also used when signal "fi.w1.wpa_supplicant1.InterfaceAdded" arrives, and in this message the properties are actually appended. This patch aims to solve this issue by distinguishing the caller to avoid trying to extract properties if they are not appended in the message. (cherry picked from commit 81eb501)
"tether" was allocated memory in tether_set_ssid() but was never freed while enabling tethering. This is the valgrind report of the issue ==14814== 12 bytes in 1 blocks are definitely lost in loss record 19 of 140 ==14814== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==14814== by 0x50CA610: g_malloc (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==14814== by 0x408989: cmd_tether (in /usr/local/bin/connmanctl) ==14814== by 0x409BEA: __connmanctl_commands (in /usr/local/bin/connmanctl) ==14814== by 0x40A0AA: rl_handler (in /usr/local/bin/connmanctl) ==14814== by 0x53AE63D: rl_callback_read_char (in /lib/x86_64-linux-gnu/libreadline.so.6.3) ==14814== by 0x40A16D: input_handler (in /usr/local/bin/connmanctl) ==14814== by 0x50C4CE4: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==14814== by 0x50C5047: g_main_context_iterate.isra.24 (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==14814== by 0x50C5309: g_main_loop_run (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==14814== by 0x40A54A: __connmanctl_input_init (in /usr/local/bin/connmanctl) ==14814== by 0x55EBEC4: (below main) (libc-start.c:287) (cherry picked from commit 00e7aaf)
Call ready callback regardless of the reply to GetManagedObjects since otherwise the user code will be left waiting forever when in fact no proxy will be created. (cherry picked from commit 06c0fd1)
If the user don't set a function it means it doesn't care about the reply so g_dbus_send_message can be used. (cherry picked from commit 2449a46)
(cherry picked from commit c47e491)
[gdbus] Ensure proxy method call frees user_data. JB#51243 Make sure that the given user_data is freed always when destroy callback is defined to avoid any memory leaks. This is because the ownership of the user_data is passed to the proxy method call, and with these changes the code conforms to rest of the codebase. Also call g_dbus_send_message() only when both function and destroy callback are not given to avoid leaking any memory.
Connectable defaults to false on every network and Sailfish OS WiFi plugin does not use it because it will be dropped as a feature later on. We need to ignore this until that to avoid networks getting removed unnecessarily.
LaakkonenJussi
force-pushed
the
connman_upgrade_to_1.33
branch
from
May 20, 2024 10:48
c155a68
to
d3af120
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.