location-service

Merge lp:~thomas-voss/location-service/fix-1441619 into lp:location-service/trunk

  1. fix-1441619
  2. Merge into trunk
Proposed by Thomas Voß on 2015-04-16
Status: Merged
Merged at revision: 191
Proposed branch: lp:~thomas-voss/location-service/fix-1441619
Merge into: lp:location-service/trunk
Diff against target: 102 lines (+20/-11)
2 files modified
src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp (+19/-10)
src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h (+1/-1)
To merge this branch: bzr merge lp:~thomas-voss/location-service/fix-1441619
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2015-04-20
Loïc Minier 2015-04-16 Approve on 2015-04-17
Review via email: mp+256460@code.launchpad.net

Commit Message

Make sure that cached modems are considered as well when calculating connection characteristics.

Description of the Change

Make sure that cached modems are considered as well when calculating connection characteristics.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Loïc Minier (lool) wrote :

Tests failed, it seems it's due to missing dbus-daemon in the environment; perhaps this needs to be an explicit build-dep?

Thomas Voß (thomas-voss) wrote :

> Tests failed, it seems it's due to missing dbus-daemon in the environment;
> perhaps this needs to be an explicit build-dep?

Nope, that is due to the dbus-daemon executable having been moved from /bin/dbus-daemon to /usr/bin/dbus-daemon. See https://code.launchpad.net/~thomas-voss/dbus-cpp/dbus-daemon-has-moved, silo'd and waiting to land.

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Loïc Minier (lool) wrote :

LGTM, except I didn't understand why:
characteristics = characteristics | all_characteristics();
is dropped.

The bulk of the diff is renaming.

Loïc Minier (lool) wrote :

14:53 < lool> tvoss: the only thing I didn't get is "characteristics = characteristics | all_characteristics();"
14:53 < lool> why would you drop that?
14:53 < tvoss> lool, it is not really needed there, and all_characteristics is a lie to a certain degree

Loïc Minier (lool) wrote :

LGTM

NB: haven't tested the binaries

review: Approve
lp:~thomas-voss/location-service/fix-1441619 updated on 2015-04-20
185. By Thomas Voß on 2015-04-20

Reenable setting all remaining flags just to be sure.

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp'
2--- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp 2015-01-06 12:35:02 +0000
3+++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp 2015-04-20 10:36:13 +0000
4@@ -75,8 +75,9 @@
5 {
6 std::lock_guard<std::mutex> lg(d.cached.guard);
7
8- for (const auto& pair : d.cached.wireless_devices)
9- pair.second.request_scan();
10+ for (const auto& pair : d.cached.nm_devices)
11+ if (pair.second.type() == xdg::NetworkManager::Device::Type::wifi)
12+ pair.second.request_scan();
13 }
14
15 const core::Signal<>& connectivity::OfonoNmConnectivityManager::wireless_network_scan_finished() const
16@@ -506,16 +507,23 @@
17 const core::dbus::types::ObjectPath& device_path,
18 std::unique_lock<std::mutex>& ul)
19 {
20- if (cached.wireless_devices.count(device_path) > 0)
21+ if (cached.nm_devices.count(device_path) > 0)
22 return;
23
24 auto device = network_manager->device_for_path(device_path);
25
26+ if (device.type() == xdg::NetworkManager::Device::Type::modem)
27+ {
28+ // Make the device known to the cache.
29+ std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device>::iterator it;
30+ std::tie(it, std::ignore) = cached.nm_devices.insert(std::make_pair(device_path, device));
31+ }
32+
33 if (device.type() == xdg::NetworkManager::Device::Type::wifi)
34 {
35 // Make the device known to the cache.
36 std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device>::iterator it;
37- std::tie(it, std::ignore) = cached.wireless_devices.insert(std::make_pair(device_path, device));
38+ std::tie(it, std::ignore) = cached.nm_devices.insert(std::make_pair(device_path, device));
39
40 // Iterate over all currently known wifis
41 it->second.for_each_access_point([this, device_path, &ul](const core::dbus::types::ObjectPath& path)
42@@ -567,7 +575,7 @@
43 const core::dbus::types::ObjectPath& path,
44 std::unique_lock<std::mutex>&)
45 {
46- cached.wireless_devices.erase(path);
47+ cached.nm_devices.erase(path);
48 }
49
50 void connectivity::OfonoNmConnectivityManager::Private::on_access_point_added(
51@@ -577,8 +585,8 @@
52 {
53 // Let's see if we have a device known for the path. We return early
54 // if we do not know about the device.
55- auto itd = cached.wireless_devices.find(device_path);
56- if (itd == cached.wireless_devices.end())
57+ auto itd = cached.nm_devices.find(device_path);
58+ if (itd == cached.nm_devices.end() || itd->second.type() != xdg::NetworkManager::Device::Type::wifi)
59 return;
60
61 xdg::NetworkManager::AccessPoint ap
62@@ -638,8 +646,8 @@
63 // We only consider cached devices and do not reach out to enumerate all of the devices
64 // to prevent from excessive dbus roundtrips.
65 std::lock_guard<std::mutex> lg{cached.guard};
66- if (cached.wireless_devices.count(path) > 0)
67- type = cached.wireless_devices.at(path).type();
68+ if (cached.nm_devices.count(path) > 0 )
69+ type = cached.nm_devices.at(path).type();
70 }
71
72 // We interpret a primary connection over a modem device as
73@@ -672,7 +680,7 @@
74 }
75 }
76
77- characteristics = characteristics | all_characteristics();
78+ characteristics = characteristics | all_characteristics();
79 } else if (type == xdg::NetworkManager::Device::Type::wifi)
80 {
81 characteristics = characteristics | connectivity::Characteristics::connection_goes_via_wifi;
82@@ -684,6 +692,7 @@
83 // Empty on purpose.
84 }
85
86+ LOG(INFO) << characteristics << std::endl;
87 return characteristics;
88 }
89
90
91=== modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h'
92--- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h 2014-11-10 20:12:56 +0000
93+++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h 2015-04-20 10:36:13 +0000
94@@ -144,7 +144,7 @@
95 std::map<core::dbus::types::ObjectPath, detail::CachedRadioCell::Ptr> cells;
96 std::map<core::dbus::types::ObjectPath, org::Ofono::Manager::Modem> modems;
97 std::map<core::dbus::types::ObjectPath, detail::CachedWirelessNetwork::Ptr> wifis;
98- std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device> wireless_devices;
99+ std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device> nm_devices;
100 std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::ActiveConnection> primary_connection;
101 std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device> primary_connection_devices;
102 } cached;

Subscribers

People subscribed via source and target branches