Merge lp:~tribaal/landscape-client/remove-hal-dbus-code into lp:~landscape/landscape-client/trunk
- remove-hal-dbus-code
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Chris Glass |
Approved revision: | 688 |
Merged at revision: | 677 |
Proposed branch: | lp:~tribaal/landscape-client/remove-hal-dbus-code |
Merge into: | lp:~landscape/landscape-client/trunk |
Diff against target: |
1140 lines (+264/-722) 9 files modified
landscape/hal.py (+0/-52) landscape/lib/disk.py (+62/-0) landscape/lib/tests/test_disk.py (+180/-1) landscape/monitor/config.py (+1/-1) landscape/monitor/hardwareinventory.py (+0/-114) landscape/monitor/mountinfo.py (+4/-92) landscape/monitor/tests/test_hardwareinventory.py (+0/-273) landscape/monitor/tests/test_mountinfo.py (+17/-102) landscape/tests/test_hal.py (+0/-87) |
To merge this branch: | bzr merge lp:~tribaal/landscape-client/remove-hal-dbus-code |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Free Ekanayaka (community) | Approve | ||
Christopher Armstrong (community) | Approve | ||
Review via email: mp+163465@code.launchpad.net |
Commit message
Removed hardwareinventory plugin, changed mountinfo plugin to not rely on HAL anymore but look at the filesystem to determine which disks are removable.
Description of the change
(Big branch, but it's mostly some deleted code)
The core problem was that the old hardwareinventory plugin and the mountinfo plugins used HAL via dbus, which is now deprecated (it was deprecated in quantal, and has been removed in raring).
- Since the hardwareinventory plugin was dead code, I removed it completely (we use the more powerful hardwareinfo manager plugin now, and that's based on lshw).
- The mountinfo plugin used the HAL to determine whether a disk is removable or not. Since the replacement API for HAL with regards to disks (the "udisk" project) seems like a moving target and differs from the HAL interface just enough to require a rewrite of some of that code, I decided to drop the dbus approach altogether and rewrite the "is this disk removable" code by simply looking at files like /sys/block/
On a side note, it seems that the kernel still struggles to determine which disks are removable sometimes (SD cards show up as non-removable for instance).
Chris Glass (tribaal) wrote : | # |
All comments should be addressed. Thanks!
Christopher Armstrong (radix) wrote : | # |
I'm still seeing test failures.
=======
[FAIL]
Traceback (most recent call last):
File "/home/
result = test_method()
File "/home/
self.
File "/usr/lib/
return original_
File "/usr/lib/
raise self.failureExc
twisted.
landscape.
=======
[ERROR]
Traceback (most recent call last):
File "/home/
self.
File "/home/
raise AssertionError(
exceptions.
=> remote_
- Performed fewer times than expected.
landscape.
=======
[ERROR]
Traceback (most recent call last):
File "/home/
HelperTestC
File "/home/
helper.
File "/home/
raise LoggedErrorsErr
landscape.
'NoneType' object has no attribute 'groups'
Traceback (most recent call last):
File "/home/
call[
File "/home/
f(*args, **kwargs)
File "/home/
for mount_info in self._get_
File "/home/
not is_device_
File "/home/
path = _get_device_
File "/home/
device_name = matched.groups()[0]
AttributeError: 'NoneType' object has no attribute 'groups'
landscape.
landscape.
Christopher Armstrong (radix) wrote : | # |
/proc/mounts
rootfs / rootfs rw 0 0
/dev/disk/
proc /proc proc rw,nosuid,
sysfs /sys sysfs rw,relatime 0 0
/dev/disk/
devpts /dev/lxc/console devpts rw,nosuid,
devpts /dev/lxc/tty1 devpts rw,nosuid,
devpts /dev/lxc/tty2 devpts rw,nosuid,
devpts /dev/lxc/tty3 devpts rw,nosuid,
devpts /dev/lxc/tty4 devpts rw,nosuid,
devpts /dev/pts devpts rw,relatime,
devpts /dev/ptmx devpts rw,relatime,
none /proc/sys/
none /sys/fs/
none /sys/kernel/debug debugfs rw,relatime 0 0
none /sys/kernel/
none /run tmpfs rw,nosuid,
none /run/lock tmpfs rw,nosuid,
none /run/shm tmpfs rw,nosuid,
none /run/user tmpfs rw,nosuid,
/etc/mtab
/dev/disk/
proc /proc proc rw,noexec,
sysfs /sys sysfs rw,noexec,
devpts /dev/pts devpts rw,noexec,
/dev/disk/
devpts /dev/lxc/console devpts rw,noexec,
devpts /dev/lxc/tty1 devpts rw,noexec,
devpts /dev/lxc/tty2 devpts rw,noexec,
devpts /dev/lxc/tty3 devpts rw,noexec,
devpts /dev/lxc/tty4 devpts rw,noexec,
devpts /dev/ptmx devpts rw,relatime,
none /proc/sys/
none /sys/fs/
none /sys/kernel/debug debugfs rw 0 0
none /sys/kernel/
none /run tmpfs rw,noexec,
none /run/lock tmpfs rw,noexec,
none /run/shm tmpfs rw,nosuid,nodev 0 0
none /run/user tmpfs rw,noexec,
Christopher Armstrong (radix) wrote : | # |
[1] Just to be extra strict, I think it's best to check if the device filename *starts* with "mmcblk" instead of just doing an "in" check.
[2] likewise, in is_device_
+ if "1" in contents:
I'd prefer that to read: if contents.strip() == "1":
[3]
Christopher Armstrong (radix) wrote : | # |
sorry, accidentally clicked "Save Comment"... :P
[3] was hardwareinventory really dead? given that it was in the plugins list that would be run by default, and it seems that when the plugin runs it sends messages to the server... Anyway, I believe you, it just seems strange that it was actually actively sending messages even though it wasn't intended to be.
I love seeing this:
9 files changed, 285 insertions(+), 719 deletions(-)
:-)
Chris Glass (tribaal) wrote : | # |
Regarding [3]:
To the best of my knowledge the server did not handle its messages at all (there is no handler for the "hardware-
Free Ekanayaka (free.ekanayaka) wrote : | # |
Nice work with the docstrings, +1!
[5]
+ When passed a device in /dev, the get_device_
s/get_device_
there are several instances of this (in the test docstrings).
[6]
+ removable_mock = self.mocker.
+ removable_mock(ANY)
+ self.mocker.
+ self.mocker.
+ self.mocker.
Instead of using mocker you could add a class variable like:
class MountInfo(
is_
and then set it in the tests:
plugin.
Preview Diff
1 | === removed file 'landscape/hal.py' |
2 | --- landscape/hal.py 2012-03-06 09:17:51 +0000 |
3 | +++ landscape/hal.py 1970-01-01 00:00:00 +0000 |
4 | @@ -1,52 +0,0 @@ |
5 | -import logging |
6 | - |
7 | -from dbus import Interface, SystemBus |
8 | -from dbus.exceptions import DBusException |
9 | - |
10 | - |
11 | -class HALManager(object): |
12 | - |
13 | - def __init__(self, bus=None): |
14 | - try: |
15 | - self._bus = bus or SystemBus() |
16 | - manager = self._bus.get_object("org.freedesktop.Hal", |
17 | - "/org/freedesktop/Hal/Manager") |
18 | - except DBusException: |
19 | - logging.error("Couldn't connect to Hal via DBus") |
20 | - self._manager = None |
21 | - else: |
22 | - self._manager = Interface(manager, "org.freedesktop.Hal.Manager") |
23 | - |
24 | - def get_devices(self): |
25 | - """Returns a list of HAL devices. |
26 | - |
27 | - @note: If it wasn't possible to connect to HAL over DBus, then an |
28 | - empty list will be returned. This can happen if the HAL or DBus |
29 | - services are not running. |
30 | - """ |
31 | - if not self._manager: |
32 | - return [] |
33 | - devices = [] |
34 | - for udi in self._manager.GetAllDevices(): |
35 | - device = self._bus.get_object("org.freedesktop.Hal", udi) |
36 | - device = Interface(device, "org.freedesktop.Hal.Device") |
37 | - device = HALDevice(device) |
38 | - devices.append(device) |
39 | - return devices |
40 | - |
41 | - |
42 | -class HALDevice(object): |
43 | - |
44 | - def __init__(self, device): |
45 | - self._children = [] |
46 | - self._device = device |
47 | - self.properties = device.GetAllProperties() |
48 | - self.udi = self.properties["info.udi"] |
49 | - self.parent = None |
50 | - |
51 | - def add_child(self, device): |
52 | - self._children.append(device) |
53 | - device.parent = self |
54 | - |
55 | - def get_children(self): |
56 | - return self._children |
57 | |
58 | === modified file 'landscape/lib/disk.py' |
59 | --- landscape/lib/disk.py 2012-09-17 13:13:23 +0000 |
60 | +++ landscape/lib/disk.py 2013-05-15 16:08:24 +0000 |
61 | @@ -2,6 +2,7 @@ |
62 | |
63 | import os |
64 | import statvfs |
65 | +import re |
66 | |
67 | |
68 | # List of filesystem types authorized when generating disk use statistics. |
69 | @@ -10,6 +11,9 @@ |
70 | "xfs", "hpfs", "jfs", "ufs", "hfs", "hfsplus"]) |
71 | |
72 | |
73 | +EXTRACT_DEVICE = re.compile("([a-z]+)[0-9]*") |
74 | + |
75 | + |
76 | def get_mount_info(mounts_file, statvfs_, |
77 | filesystems_whitelist=STABLE_FILESYSTEMS): |
78 | """ |
79 | @@ -74,3 +78,61 @@ |
80 | or path_segments[:len(mount_segments)] == mount_segments): |
81 | candidate = info |
82 | return candidate |
83 | + |
84 | + |
85 | +def is_device_removable(device): |
86 | + """ |
87 | + This function returns whether a given device is removable or not by looking |
88 | + at the corresponding /sys/block/<device>/removable file |
89 | + |
90 | + @param device: The filesystem path to the device, e.g. /dev/sda1 |
91 | + """ |
92 | + # Shortcut the case where the device an SD card. The kernel/udev currently |
93 | + # consider SD cards (mmcblk devices) to be non-removable. |
94 | + if os.path.basename(device).startswith("mmcblk"): |
95 | + return True |
96 | + |
97 | + path = _get_device_removable_file_path(device) |
98 | + |
99 | + if not path: |
100 | + return False |
101 | + |
102 | + contents = None |
103 | + try: |
104 | + with open(path, "r") as f: |
105 | + contents = f.readline() |
106 | + except IOError: |
107 | + return False |
108 | + |
109 | + if contents.strip() == "1": |
110 | + return True |
111 | + return False |
112 | + |
113 | + |
114 | +def _get_device_removable_file_path(device): |
115 | + """ |
116 | + Get a device's "removable" file path. |
117 | + |
118 | + This function figures out the C{/sys/block/<device>/removable} path |
119 | + associated with the given device. The file at that path contains either |
120 | + a "0" if the device is not removable, or a "1" if it is. |
121 | + |
122 | + @param device: File system path of the device. |
123 | + """ |
124 | + # The device will be a symlink if the disk is mounted by uuid or by label. |
125 | + if os.path.islink(device): |
126 | + # Paths are in the form "/dev/disk/by-uuid/<uuid>" and symlink |
127 | + # to the device file under /dev |
128 | + device = os.readlink(device) # /dev/disk/by-uuid/<uuid> -> ../../sda1 |
129 | + |
130 | + [device_name] = device.split("/")[-1:] # /dev/sda1 -> sda1 |
131 | + |
132 | + matched = EXTRACT_DEVICE.match(device_name) # sda1 -> sda |
133 | + |
134 | + if not matched: |
135 | + return None |
136 | + |
137 | + device_name = matched.groups()[0] |
138 | + |
139 | + removable_file = os.path.join("/sys/block/", device_name, "removable") |
140 | + return removable_file |
141 | |
142 | === modified file 'landscape/lib/tests/test_disk.py' |
143 | --- landscape/lib/tests/test_disk.py 2012-09-17 13:13:23 +0000 |
144 | +++ landscape/lib/tests/test_disk.py 2013-05-15 16:08:24 +0000 |
145 | @@ -1,6 +1,8 @@ |
146 | import os |
147 | |
148 | -from landscape.lib.disk import get_filesystem_for_path, get_mount_info |
149 | +from landscape.lib.disk import ( |
150 | + get_filesystem_for_path, get_mount_info, is_device_removable, |
151 | + _get_device_removable_file_path) |
152 | from landscape.tests.helpers import LandscapeTest |
153 | |
154 | |
155 | @@ -101,3 +103,180 @@ |
156 | expected = {"device": "/dev/sda1", "mount-point": "/home", |
157 | "filesystem": "ext4", "total-space": 3, "free-space": 1} |
158 | self.assertEqual([expected], result) |
159 | + |
160 | + |
161 | +class RemovableDiskTest(LandscapeTest): |
162 | + |
163 | + def test_wb_get_device_removable_file_path(self): |
164 | + """ |
165 | + When passed a device in /dev, the L{_get_device_removable_file_path} |
166 | + function returns the corresponding removable file path in /sys/block. |
167 | + """ |
168 | + device = "/dev/sdb" |
169 | + expected = "/sys/block/sdb/removable" |
170 | + |
171 | + is_link_mock = self.mocker.replace(os.path.islink) |
172 | + is_link_mock(device) |
173 | + self.mocker.result(False) |
174 | + self.mocker.replay() |
175 | + |
176 | + result = _get_device_removable_file_path(device) |
177 | + self.assertEqual(expected, result) |
178 | + |
179 | + def test_wb_get_device_removable_file_path_with_partition(self): |
180 | + """ |
181 | + When passed a device in /dev with a partition number, the |
182 | + L{_get_device_removable_file_path} function returns the corresponding |
183 | + removable file path in /sys/block. |
184 | + """ |
185 | + device = "/dev/sdb1" |
186 | + expected = "/sys/block/sdb/removable" |
187 | + |
188 | + is_link_mock = self.mocker.replace(os.path.islink) |
189 | + is_link_mock(device) |
190 | + self.mocker.result(False) |
191 | + self.mocker.replay() |
192 | + |
193 | + result = _get_device_removable_file_path(device) |
194 | + self.assertEqual(expected, result) |
195 | + |
196 | + def test_wb_get_device_removable_file_path_without_dev(self): |
197 | + """ |
198 | + When passed a device name (not the whole path), the |
199 | + L{_get_device_removable_file_path} function returns the corresponding |
200 | + removable file path in /sys/block. |
201 | + """ |
202 | + device = "sdb1" |
203 | + expected = "/sys/block/sdb/removable" |
204 | + |
205 | + is_link_mock = self.mocker.replace(os.path.islink) |
206 | + is_link_mock(device) |
207 | + self.mocker.result(False) |
208 | + self.mocker.replay() |
209 | + |
210 | + result = _get_device_removable_file_path(device) |
211 | + self.assertEqual(expected, result) |
212 | + |
213 | + def test_wb_get_device_removable_file_path_with_symlink(self): |
214 | + """ |
215 | + When the device path passed to L{_get_device_removable_file_path} is a |
216 | + symlink (it's the case when disks are mounted by uuid or by label), |
217 | + the L{_get_device_removable_file_path} function returns the proper |
218 | + corresponding file path in /sys/block. |
219 | + """ |
220 | + device = "/dev/disk/by-uuid/8b2ec410-ebd2-49ec-bb3c-b8b13effab08" |
221 | + |
222 | + readlink_mock = self.mocker.replace(os.readlink) |
223 | + readlink_mock(device) |
224 | + self.mocker.result("../../sda1") |
225 | + |
226 | + is_link_mock = self.mocker.replace(os.path.islink) |
227 | + is_link_mock(device) |
228 | + self.mocker.result(True) |
229 | + |
230 | + self.mocker.replay() |
231 | + |
232 | + expected = "/sys/block/sda/removable" |
233 | + result = _get_device_removable_file_path(device) |
234 | + self.assertEqual(expected, result) |
235 | + |
236 | + def test_wb_get_device_removable_file_path_raid_device(self): |
237 | + """ |
238 | + When passed a more exotic device file, like for example a raid device |
239 | + (e.g. /dev/cciss/c0d1p1), the L{_get_device_removable_file_path} |
240 | + function does not fail, and returns the expected |
241 | + /sys/block/<device>/removable path. |
242 | + """ |
243 | + device = "/dev/cciss/c0d0p0" |
244 | + # The expected path does not exists, but it doesn't matter here. |
245 | + expected = "/sys/block/c/removable" |
246 | + |
247 | + is_link_mock = self.mocker.replace(os.path.islink) |
248 | + is_link_mock(device) |
249 | + self.mocker.result(False) |
250 | + |
251 | + self.mocker.replay() |
252 | + |
253 | + result = _get_device_removable_file_path(device) |
254 | + self.assertEqual(expected, result) |
255 | + |
256 | + def test_is_device_removable(self): |
257 | + """ |
258 | + Given the path to a file, determine if it means the device is removable |
259 | + or not. |
260 | + """ |
261 | + device = "/dev/sdb1" |
262 | + path = self.makeFile("1") |
263 | + |
264 | + removable_mock = self.mocker.replace(_get_device_removable_file_path) |
265 | + removable_mock(device) |
266 | + self.mocker.result(path) |
267 | + self.mocker.replay() |
268 | + |
269 | + self.assertTrue(is_device_removable(device)) |
270 | + |
271 | + def test_is_device_removable_false(self): |
272 | + """ |
273 | + Given the path to a file, determine if it means the device is removable |
274 | + or not. |
275 | + """ |
276 | + device = "/dev/sdb1" |
277 | + path = self.makeFile("0") |
278 | + |
279 | + removable_mock = self.mocker.replace(_get_device_removable_file_path) |
280 | + removable_mock(device) |
281 | + self.mocker.result(path) |
282 | + self.mocker.replay() |
283 | + |
284 | + self.assertFalse(is_device_removable(device)) |
285 | + |
286 | + def test_is_device_removable_garbage(self): |
287 | + """ |
288 | + Given the path to a file, determine if it means the device is removable |
289 | + or not. |
290 | + """ |
291 | + device = "/dev/sdb1" |
292 | + path = self.makeFile("Some garbage") |
293 | + |
294 | + removable_mock = self.mocker.replace(_get_device_removable_file_path) |
295 | + removable_mock(device) |
296 | + self.mocker.result(path) |
297 | + self.mocker.replay() |
298 | + |
299 | + self.assertFalse(is_device_removable(device)) |
300 | + |
301 | + def test_is_device_removable_path_doesnt_exist(self): |
302 | + """ |
303 | + When given a non-existing path, report the device as not removable. |
304 | + """ |
305 | + device = "/dev/sdb1" |
306 | + path = "/what/ever" |
307 | + |
308 | + removable_mock = self.mocker.replace(_get_device_removable_file_path) |
309 | + removable_mock(device) |
310 | + self.mocker.result(path) |
311 | + self.mocker.replay() |
312 | + |
313 | + self.assertFalse(is_device_removable(device)) |
314 | + |
315 | + def test_is_removable_raid_device(self): |
316 | + """ |
317 | + When passed the path to a raid device (e.g. /dev/cciss/c0d0p0), the |
318 | + is_device_removable function returns False. |
319 | + """ |
320 | + device = "/dev/cciss/c0d1p1" |
321 | + |
322 | + is_link_mock = self.mocker.replace(os.path.islink) |
323 | + is_link_mock(device) |
324 | + self.mocker.result(False) |
325 | + self.mocker.replay() |
326 | + |
327 | + self.assertFalse(is_device_removable(device)) |
328 | + |
329 | + def test_is_device_removable_memory_card(self): |
330 | + """ |
331 | + The kernel/udev currently consider memory cards such as SD cards as non |
332 | + removable |
333 | + """ |
334 | + device = "/dev/mmcblk0p1" # Device 0, parition 1 |
335 | + self.assertTrue(is_device_removable(device)) |
336 | |
337 | === modified file 'landscape/monitor/config.py' |
338 | --- landscape/monitor/config.py 2013-01-28 12:46:54 +0000 |
339 | +++ landscape/monitor/config.py 2013-05-15 16:08:24 +0000 |
340 | @@ -1,7 +1,7 @@ |
341 | from landscape.deployment import Configuration |
342 | |
343 | |
344 | -ALL_PLUGINS = ["ActiveProcessInfo", "ComputerInfo", "HardwareInventory", |
345 | +ALL_PLUGINS = ["ActiveProcessInfo", "ComputerInfo", |
346 | "LoadAverage", "MemoryInfo", "MountInfo", "ProcessorInfo", |
347 | "Temperature", "PackageMonitor", "UserMonitor", |
348 | "RebootRequired", "AptPreferences", "NetworkActivity", |
349 | |
350 | === removed file 'landscape/monitor/hardwareinventory.py' |
351 | --- landscape/monitor/hardwareinventory.py 2012-03-05 14:11:42 +0000 |
352 | +++ landscape/monitor/hardwareinventory.py 1970-01-01 00:00:00 +0000 |
353 | @@ -1,114 +0,0 @@ |
354 | -import logging |
355 | - |
356 | -from twisted.internet.defer import succeed |
357 | - |
358 | -from landscape.lib.log import log_failure |
359 | - |
360 | -from landscape.diff import diff |
361 | -from landscape.monitor.plugin import MonitorPlugin |
362 | - |
363 | - |
364 | -class HardwareInventory(MonitorPlugin): |
365 | - |
366 | - persist_name = "hardware-inventory" |
367 | - |
368 | - def __init__(self, hal_manager=None): |
369 | - super(HardwareInventory, self).__init__() |
370 | - self._persist_sets = [] |
371 | - self._persist_removes = [] |
372 | - self.enabled = True |
373 | - try: |
374 | - from landscape.hal import HALManager |
375 | - except ImportError: |
376 | - self.enabled = False |
377 | - else: |
378 | - self._hal_manager = hal_manager or HALManager() |
379 | - |
380 | - def register(self, manager): |
381 | - if not self.enabled: |
382 | - return |
383 | - super(HardwareInventory, self).register(manager) |
384 | - self.call_on_accepted("hardware-inventory", self.exchange, True) |
385 | - |
386 | - def send_message(self, urgent): |
387 | - devices = self.create_message() |
388 | - if devices: |
389 | - message = {"type": "hardware-inventory", "devices": devices} |
390 | - result = self.registry.broker.send_message(message, urgent=urgent) |
391 | - result.addCallback(self.persist_data) |
392 | - result.addErrback(log_failure) |
393 | - logging.info("Queueing a message with hardware-inventory " |
394 | - "information.") |
395 | - else: |
396 | - result = succeed(None) |
397 | - return result |
398 | - |
399 | - def exchange(self, urgent=False): |
400 | - if not self.enabled: |
401 | - return |
402 | - return self.registry.broker.call_if_accepted("hardware-inventory", |
403 | - self.send_message, urgent) |
404 | - |
405 | - def persist_data(self, message_id): |
406 | - for key, udi, value in self._persist_sets: |
407 | - self._persist.set((key, udi), value) |
408 | - for key in self._persist_removes: |
409 | - self._persist.remove(key) |
410 | - del self._persist_sets[:] |
411 | - del self._persist_removes[:] |
412 | - # This forces the registry to write the persistent store to disk |
413 | - # This means that the persistent data reflects the state of the |
414 | - # messages sent. |
415 | - self.registry.flush() |
416 | - |
417 | - def create_message(self): |
418 | - # FIXME Using persist to keep track of changes here uses a |
419 | - # fair amount of memory. On my machine a rough test seemed to |
420 | - # indicate that memory usage grew by 1.3mb, about 12% of the |
421 | - # overall process size. Look here to save memory. |
422 | - del self._persist_sets[:] |
423 | - del self._persist_removes[:] |
424 | - devices = [] |
425 | - previous_devices = self._persist.get("devices", {}) |
426 | - current_devices = set() |
427 | - |
428 | - for device in self._hal_manager.get_devices(): |
429 | - previous_properties = previous_devices.get(device.udi) |
430 | - if not previous_properties: |
431 | - devices.append(("create", device.properties)) |
432 | - elif previous_properties != device.properties: |
433 | - creates, updates, deletes = diff(previous_properties, |
434 | - device.properties) |
435 | - devices.append(("update", device.udi, |
436 | - creates, updates, deletes)) |
437 | - current_devices.add(device.udi) |
438 | - self._persist_sets.append( |
439 | - ("devices", device.udi, device.properties)) |
440 | - |
441 | - items_with_parents = {} |
442 | - deleted_devices = set() |
443 | - for udi, value in previous_devices.iteritems(): |
444 | - if udi not in current_devices: |
445 | - if "info.parent" in value: |
446 | - items_with_parents[udi] = value["info.parent"] |
447 | - deleted_devices.add(udi) |
448 | - |
449 | - # We remove the deleted devices from our persistent store it's |
450 | - # only the information we're sending to the server that we're |
451 | - # compressing. |
452 | - for udi in deleted_devices: |
453 | - self._persist_removes.append(("devices", udi)) |
454 | - |
455 | - # We can now flatten the list of devices we send to the server |
456 | - # For each of the items_with_parents, if both the item and it's parent |
457 | - # are in the deleted_devices set, then we can remove this item from the |
458 | - # set. |
459 | - minimal_deleted_devices = deleted_devices.copy() |
460 | - for child, parent in items_with_parents.iteritems(): |
461 | - if child in deleted_devices and parent in deleted_devices: |
462 | - minimal_deleted_devices.remove(child) |
463 | - # We now build the deleted devices message |
464 | - for udi in minimal_deleted_devices: |
465 | - devices.append(("delete", udi)) |
466 | - |
467 | - return devices |
468 | |
469 | === modified file 'landscape/monitor/mountinfo.py' |
470 | --- landscape/monitor/mountinfo.py 2013-01-23 20:50:19 +0000 |
471 | +++ landscape/monitor/mountinfo.py 2013-05-15 16:08:24 +0000 |
472 | @@ -1,7 +1,7 @@ |
473 | import time |
474 | import os |
475 | |
476 | -from landscape.lib.disk import get_mount_info |
477 | +from landscape.lib.disk import get_mount_info, is_device_removable |
478 | from landscape.lib.monitor import CoverageMonitor |
479 | from landscape.accumulate import Accumulator |
480 | from landscape.monitor.plugin import MonitorPlugin |
481 | @@ -15,7 +15,7 @@ |
482 | |
483 | def __init__(self, interval=300, monitor_interval=60 * 60, |
484 | mounts_file="/proc/mounts", create_time=time.time, |
485 | - statvfs=None, hal_manager=None, mtab_file="/etc/mtab"): |
486 | + statvfs=None, mtab_file="/etc/mtab"): |
487 | self.run_interval = interval |
488 | self._monitor_interval = monitor_interval |
489 | self._create_time = create_time |
490 | @@ -28,22 +28,7 @@ |
491 | self._free_space = [] |
492 | self._mount_info = [] |
493 | self._mount_info_to_persist = None |
494 | - try: |
495 | - from landscape.hal import HALManager |
496 | - except ImportError: |
497 | - self._hal_manager = hal_manager |
498 | - else: |
499 | - self._hal_manager = hal_manager or HALManager() |
500 | - try: |
501 | - from gi.repository import GUdev |
502 | - except ImportError: |
503 | - self._gudev_client = None |
504 | - else: |
505 | - try: |
506 | - self._gudev_client = GUdev.Client.new([]) |
507 | - except AttributeError: |
508 | - # gudev < 1.2 uses a different unsupported interface |
509 | - self._gudev_client = None |
510 | + self.is_device_removable = is_device_removable |
511 | |
512 | def register(self, registry): |
513 | super(MountInfo, self).register(registry) |
514 | @@ -121,81 +106,8 @@ |
515 | |
516 | current_mount_points.add(mount_point) |
517 | |
518 | - def _get_removable_devices(self): |
519 | - if self._hal_manager is not None: |
520 | - return self._get_hal_removable_devices() |
521 | - elif self._gudev_client is not None: |
522 | - return self._get_udev_removable_devices() |
523 | - else: |
524 | - return set() |
525 | - |
526 | - def _get_udev_removable_devices(self): |
527 | - class is_removable(object): |
528 | - def __contains__(oself, device_name): |
529 | - device = self._gudev_client.query_by_device_file(device_name) |
530 | - if device: |
531 | - return device.get_sysfs_attr_as_boolean("removable") |
532 | - return False |
533 | - return is_removable() |
534 | - |
535 | - def _get_hal_removable_devices(self): |
536 | - block_devices = {} # {udi: [device, ...]} |
537 | - children = {} # {parent_udi: [child_udi, ...]} |
538 | - removable = set() |
539 | - |
540 | - # We walk the list of devices building up a dictionary of all removable |
541 | - # devices, and a mapping of {UDI => [block devices]} |
542 | - # We differentiate between devices that we definitely know are |
543 | - # removable and devices that _may_ be removable, depending on their |
544 | - # parent device, e.g. /dev/sdb1 isn't flagged as removable, but |
545 | - # /dev/sdb may well be removable. |
546 | - |
547 | - # Unfortunately, HAL doesn't guarantee the order of the devices |
548 | - # returned from get_devices(), so we may not know that a parent device |
549 | - # is removable when we find it's first child. |
550 | - devices = self._hal_manager.get_devices() |
551 | - for device in devices: |
552 | - block_device = device.properties.get("block.device") |
553 | - if block_device: |
554 | - if device.properties.get("storage.removable"): |
555 | - removable.add(device.udi) |
556 | - |
557 | - try: |
558 | - block_devices[device.udi].append(block_device) |
559 | - except KeyError: |
560 | - block_devices[device.udi] = [block_device] |
561 | - |
562 | - parent_udi = device.properties.get("info.parent") |
563 | - if parent_udi is not None: |
564 | - try: |
565 | - children[parent_udi].append(device.udi) |
566 | - except KeyError: |
567 | - children[parent_udi] = [device.udi] |
568 | - |
569 | - # Propagate the removable flag from each node all the way to |
570 | - # its leaf children. |
571 | - updated = True |
572 | - while updated: |
573 | - updated = False |
574 | - for parent_udi in children: |
575 | - if parent_udi in removable: |
576 | - for child_udi in children[parent_udi]: |
577 | - if child_udi not in removable: |
578 | - removable.add(child_udi) |
579 | - updated = True |
580 | - |
581 | - # We've now seen _all_ devices, and have the definitive list of |
582 | - # removable UDIs, so we can now find all the removable devices in the |
583 | - # system. |
584 | - removable_devices = set() |
585 | - for udi in removable: |
586 | - removable_devices.update(block_devices[udi]) |
587 | - |
588 | - return removable_devices |
589 | - |
590 | def _get_mount_info(self): |
591 | """Generator yields local mount points worth recording data for.""" |
592 | - removable_devices = self._get_removable_devices() |
593 | bound_mount_points = self._get_bound_mount_points() |
594 | |
595 | for info in get_mount_info(self._mounts_file, self._statvfs): |
596 | @@ -203,7 +115,7 @@ |
597 | mount_point = info["mount-point"] |
598 | if (device.startswith("/dev/") and |
599 | not mount_point.startswith("/dev/") and |
600 | - not device in removable_devices and |
601 | + not self.is_device_removable(device) and |
602 | not mount_point in bound_mount_points): |
603 | yield info |
604 | |
605 | |
606 | === removed file 'landscape/monitor/tests/test_hardwareinventory.py' |
607 | --- landscape/monitor/tests/test_hardwareinventory.py 2012-03-05 14:11:42 +0000 |
608 | +++ landscape/monitor/tests/test_hardwareinventory.py 1970-01-01 00:00:00 +0000 |
609 | @@ -1,273 +0,0 @@ |
610 | -from twisted.internet.defer import fail, succeed |
611 | - |
612 | -from landscape.monitor.hardwareinventory import HardwareInventory |
613 | -from landscape.tests.test_hal import MockHALManager, MockRealHALDevice |
614 | -from landscape.tests.helpers import LandscapeTest, MonitorHelper |
615 | -from landscape.tests.mocker import ANY |
616 | -from landscape.message_schemas import HARDWARE_INVENTORY |
617 | - |
618 | - |
619 | -class HardwareInventoryTest(LandscapeTest): |
620 | - |
621 | - helpers = [MonitorHelper] |
622 | - |
623 | - def setUp(self): |
624 | - super(HardwareInventoryTest, self).setUp() |
625 | - self.mstore.set_accepted_types(["hardware-inventory"]) |
626 | - devices = [MockRealHALDevice({u"info.udi": u"wubble", |
627 | - u"info.product": u"Wubble"}), |
628 | - MockRealHALDevice({u"info.udi": u"ooga", |
629 | - u"info.product": u"Ooga"})] |
630 | - self.hal_manager = MockHALManager(devices) |
631 | - self.plugin = HardwareInventory(hal_manager=self.hal_manager) |
632 | - self.monitor.add(self.plugin) |
633 | - |
634 | - def assertSchema(self, devices): |
635 | - full_message = {"type": "hardware-inventory", "devices": devices} |
636 | - self.assertEqual(HARDWARE_INVENTORY.coerce(full_message), |
637 | - full_message) |
638 | - |
639 | - def test_hal_devices(self): |
640 | - """ |
641 | - The first time the plugin runs it should report information |
642 | - about all HAL devices found on the system. Every UDI provided |
643 | - by HAL should be present in the devices list as is from HAL. |
644 | - """ |
645 | - message = self.plugin.create_message() |
646 | - actual_udis = [part[1][u"info.udi"] for part in message] |
647 | - expected_udis = [device.udi for device |
648 | - in self.hal_manager.get_devices()] |
649 | - self.assertEqual(set(actual_udis), set(expected_udis)) |
650 | - |
651 | - def test_first_message(self): |
652 | - """ |
653 | - The first time the plugin runs it should report information |
654 | - about all HAL devices found on the system. All new devices |
655 | - will be reported with 'create' actions. |
656 | - """ |
657 | - message = self.plugin.create_message() |
658 | - actions = [part[0] for part in message] |
659 | - self.assertEqual(set(actions), set(["create"])) |
660 | - self.assertSchema(message) |
661 | - |
662 | - def test_no_changes(self): |
663 | - """ |
664 | - Messages should not be created if hardware information is |
665 | - unchanged since the last server exchange. |
666 | - """ |
667 | - self.plugin.exchange() |
668 | - self.assertNotEquals(len(self.mstore.get_pending_messages()), 0) |
669 | - |
670 | - messages = self.mstore.get_pending_messages() |
671 | - self.plugin.exchange() |
672 | - self.assertEqual(self.mstore.get_pending_messages(), messages) |
673 | - |
674 | - def test_update(self): |
675 | - """ |
676 | - If a change is detected for a device that was previously |
677 | - reported to the server, the changed device should be reported |
678 | - with an 'update' action. Property changes are reported at a |
679 | - key/value pair level. |
680 | - """ |
681 | - self.hal_manager.devices = [ |
682 | - MockRealHALDevice({u"info.udi": u"wubble", |
683 | - u"info.product": u"Wubble"})] |
684 | - registry_mocker = self.mocker.replace(self.plugin.registry) |
685 | - registry_mocker.flush() |
686 | - self.mocker.count(2) |
687 | - self.mocker.result(None) |
688 | - self.mocker.replay() |
689 | - message = self.plugin.create_message() |
690 | - self.plugin.persist_data(None) |
691 | - self.assertEqual(message, [("create", {u"info.udi": u"wubble", |
692 | - u"info.product": u"Wubble"})]) |
693 | - |
694 | - self.hal_manager.devices[0] = MockRealHALDevice( |
695 | - {u"info.udi": u"wubble", u"info.product": u"Ooga"}) |
696 | - message = self.plugin.create_message() |
697 | - self.plugin.persist_data(None) |
698 | - self.assertEqual(message, [("update", u"wubble", |
699 | - {}, {u"info.product": u"Ooga"}, {})]) |
700 | - self.assertSchema(message) |
701 | - self.assertEqual(self.plugin.create_message(), []) |
702 | - |
703 | - def test_update_list(self): |
704 | - """ |
705 | - An update should be sent to the server when a strlist device |
706 | - property changes. No updates should be sent if a device is |
707 | - unchanged. |
708 | - """ |
709 | - self.hal_manager.devices = [ |
710 | - MockRealHALDevice({u"info.udi": u"wubble", |
711 | - u"info.product": u"Wubble", |
712 | - u"info.capabilities": [u"foo", u"bar"]})] |
713 | - |
714 | - message = self.plugin.create_message() |
715 | - self.plugin.persist_data(None) |
716 | - self.assertEqual(message, [("create", |
717 | - {u"info.udi": u"wubble", |
718 | - u"info.product": u"Wubble", |
719 | - u"info.capabilities": [u"foo", u"bar"]}), |
720 | - ]) |
721 | - |
722 | - self.assertSchema(message) |
723 | - |
724 | - self.hal_manager.devices[0] = MockRealHALDevice( |
725 | - {u"info.udi": u"wubble", u"info.product": u"Wubble", |
726 | - u"info.capabilities": [u"foo"]}) |
727 | - message = self.plugin.create_message() |
728 | - self.plugin.persist_data(None) |
729 | - self.assertEqual(message, [("update", u"wubble", |
730 | - {}, {u"info.capabilities": [u"foo"]}, {}), |
731 | - ]) |
732 | - self.assertSchema(message) |
733 | - |
734 | - self.assertEqual(self.plugin.create_message(), []) |
735 | - |
736 | - def test_update_complex(self): |
737 | - """ |
738 | - The 'update' action reports property create, update and |
739 | - delete changes. |
740 | - """ |
741 | - self.hal_manager.devices = [ |
742 | - MockRealHALDevice({u"info.udi": u"wubble", |
743 | - u"info.product": u"Wubble", |
744 | - u"linux.acpi_type": 11})] |
745 | - |
746 | - message = self.plugin.create_message() |
747 | - self.plugin.persist_data(None) |
748 | - self.assertEqual(message, [("create", {u"info.udi": u"wubble", |
749 | - u"info.product": u"Wubble", |
750 | - u"linux.acpi_type": 11})]) |
751 | - |
752 | - self.hal_manager.devices[0] = MockRealHALDevice( |
753 | - {u"info.udi": u"wubble", u"info.product": u"Ooga", |
754 | - u"info.category": u"unittest"}) |
755 | - message = self.plugin.create_message() |
756 | - self.plugin.persist_data(None) |
757 | - self.assertEqual(message, [("update", u"wubble", |
758 | - {u"info.category": u"unittest"}, |
759 | - {u"info.product": u"Ooga"}, |
760 | - {u"linux.acpi_type": 11})]) |
761 | - self.assertSchema(message) |
762 | - |
763 | - self.assertEqual(self.plugin.create_message(), []) |
764 | - |
765 | - def test_delete(self): |
766 | - """ |
767 | - If a device that was previously reported is no longer present |
768 | - in a system a device entry should be created with a 'delete' |
769 | - action. |
770 | - """ |
771 | - self.hal_manager.devices = [ |
772 | - MockRealHALDevice({u"info.udi": u"wubble", |
773 | - u"info.product": u"Wubble"}), |
774 | - MockRealHALDevice({u"info.udi": u"ooga", |
775 | - u"info.product": u"Ooga"})] |
776 | - |
777 | - message = self.plugin.create_message() |
778 | - self.plugin.persist_data(None) |
779 | - self.assertEqual(message, [("create", {u"info.udi": u"wubble", |
780 | - u"info.product": u"Wubble"}), |
781 | - ("create", {u"info.udi": u"ooga", |
782 | - u"info.product": u"Ooga"})]) |
783 | - self.assertSchema(message) |
784 | - |
785 | - self.hal_manager.devices.pop(1) |
786 | - message = self.plugin.create_message() |
787 | - self.plugin.persist_data(None) |
788 | - self.assertEqual(message, [("delete", u"ooga")]) |
789 | - self.assertSchema(message) |
790 | - self.assertEqual(self.plugin.create_message(), []) |
791 | - |
792 | - def test_minimal_delete(self): |
793 | - self.hal_manager.devices = [ |
794 | - MockRealHALDevice({u"info.udi": u"wubble", |
795 | - u"block.device": u"/dev/scd", |
796 | - u"storage.removable": True}), |
797 | - MockRealHALDevice({u"info.udi": u"wubble0", |
798 | - u"block.device": u"/dev/scd0", |
799 | - u"info.parent": u"wubble"}), |
800 | - MockRealHALDevice({u"info.udi": u"wubble1", |
801 | - u"block.device": u"/dev/scd1", |
802 | - u"info.parent": u"wubble"}), |
803 | - MockRealHALDevice({u"info.udi": u"wubble2", |
804 | - u"block.device": u"/dev/scd1", |
805 | - u"info.parent": u"wubble0"}), |
806 | - MockRealHALDevice({u"info.udi": u"wubble3", |
807 | - u"block.device": u"/dev/scd1", |
808 | - u"info.parent": u"wubble2"})] |
809 | - |
810 | - message = self.plugin.create_message() |
811 | - self.plugin.persist_data(None) |
812 | - |
813 | - del self.hal_manager.devices[:] |
814 | - |
815 | - message = self.plugin.create_message() |
816 | - self.plugin.persist_data(None) |
817 | - |
818 | - self.assertEqual(message, [("delete", u"wubble")]) |
819 | - self.assertEqual(self.plugin.create_message(), []) |
820 | - |
821 | - def test_resynchronize(self): |
822 | - """ |
823 | - If a 'resynchronize' reactor event is fired, the plugin should |
824 | - send a message that contains all data as if the server has |
825 | - none. |
826 | - """ |
827 | - self.plugin.exchange() |
828 | - self.reactor.fire("resynchronize") |
829 | - self.plugin.exchange() |
830 | - |
831 | - messages = self.mstore.get_pending_messages() |
832 | - self.assertEqual(len(messages), 2) |
833 | - self.assertEqual(messages[0]["devices"], messages[1]["devices"]) |
834 | - |
835 | - def test_call_on_accepted(self): |
836 | - remote_broker_mock = self.mocker.replace(self.remote) |
837 | - remote_broker_mock.send_message(ANY, urgent=True) |
838 | - self.mocker.result(succeed(None)) |
839 | - self.mocker.replay() |
840 | - |
841 | - self.reactor.fire(("message-type-acceptance-changed", |
842 | - "hardware-inventory"), |
843 | - True) |
844 | - |
845 | - def test_no_message_if_not_accepted(self): |
846 | - """ |
847 | - Don't add any messages at all if the broker isn't currently |
848 | - accepting their type. |
849 | - """ |
850 | - self.mstore.set_accepted_types([]) |
851 | - self.reactor.advance(self.monitor.step_size * 2) |
852 | - self.monitor.exchange() |
853 | - |
854 | - self.mstore.set_accepted_types(["hardware-inventory"]) |
855 | - self.assertMessages(list(self.mstore.get_pending_messages()), []) |
856 | - |
857 | - def test_do_not_persist_changes_when_send_message_fails(self): |
858 | - """ |
859 | - When the plugin is run it persists data that it uses on |
860 | - subsequent checks to calculate the delta to send. It should |
861 | - only persist data when the broker confirms that the message |
862 | - sent by the plugin has been sent. |
863 | - """ |
864 | - |
865 | - class MyException(Exception): |
866 | - pass |
867 | - |
868 | - self.log_helper.ignore_errors(MyException) |
869 | - |
870 | - broker_mock = self.mocker.replace(self.monitor.broker) |
871 | - broker_mock.send_message(ANY, urgent=ANY) |
872 | - self.mocker.result(fail(MyException())) |
873 | - self.mocker.replay() |
874 | - |
875 | - message = self.plugin.create_message() |
876 | - |
877 | - def assert_message(message_id): |
878 | - self.assertEqual(message, self.plugin.create_message()) |
879 | - |
880 | - result = self.plugin.exchange() |
881 | - result.addCallback(assert_message) |
882 | - return result |
883 | |
884 | === modified file 'landscape/monitor/tests/test_mountinfo.py' |
885 | --- landscape/monitor/tests/test_mountinfo.py 2013-01-23 20:50:19 +0000 |
886 | +++ landscape/monitor/tests/test_mountinfo.py 2013-05-15 16:08:24 +0000 |
887 | @@ -3,7 +3,6 @@ |
888 | from twisted.internet.defer import succeed |
889 | |
890 | from landscape.monitor.mountinfo import MountInfo |
891 | -from landscape.tests.test_hal import MockHALManager, MockRealHALDevice |
892 | from landscape.tests.helpers import LandscapeTest, mock_counter, MonitorHelper |
893 | from landscape.tests.mocker import ANY |
894 | |
895 | @@ -22,11 +21,12 @@ |
896 | self.log_helper.ignore_errors("Typelib file for namespace") |
897 | |
898 | def get_mount_info(self, *args, **kwargs): |
899 | - hal_devices = kwargs.pop("hal_devices", []) |
900 | - kwargs["hal_manager"] = MockHALManager(hal_devices) |
901 | if "statvfs" not in kwargs: |
902 | - kwargs["statvfs"] = lambda path: (0,) * 10 |
903 | - return MountInfo(*args, **kwargs) |
904 | + kwargs["statvfs"] = lambda path: (0,) * 1000 |
905 | + plugin = MountInfo(*args, **kwargs) |
906 | + # To make sure tests are isolated from the real system by default. |
907 | + plugin.is_device_removable = lambda x: False |
908 | + return plugin |
909 | |
910 | def test_read_proc_mounts(self): |
911 | """ |
912 | @@ -98,7 +98,6 @@ |
913 | /dev/hde1 /mnt/bind none rw,bind 0 0 |
914 | /dev/sdb2 /media/Boot\\040OSX hfsplus rw 0 0 |
915 | """) |
916 | - |
917 | plugin = self.get_mount_info(mounts_file=filename, statvfs=statvfs, |
918 | create_time=self.reactor.time, |
919 | mtab_file=mtab_filename) |
920 | @@ -325,102 +324,16 @@ |
921 | |
922 | def test_ignore_removable_partitions(self): |
923 | """ |
924 | - Partitions on removable devices don't directly report |
925 | - storage.removable : True, but they do point to their parent and the |
926 | - parent will be marked removable if appropriate. |
927 | - """ |
928 | - devices = [MockRealHALDevice({"info.udi": "wubble", |
929 | - "block.device": "/dev/scd", |
930 | - "storage.removable": True}), |
931 | - MockRealHALDevice({"info.udi": "wubble0", |
932 | - "block.device": "/dev/scd0", |
933 | - "info.parent": "wubble"})] |
934 | - |
935 | - filename = self.makeFile("""\ |
936 | -/dev/scd0 /media/Xerox_M750 iso9660 ro,nosuid,nodev,uid=1000,utf8 0 0 |
937 | -""") |
938 | - plugin = self.get_mount_info(mounts_file=filename, hal_devices=devices, |
939 | - mtab_file=filename) |
940 | - self.monitor.add(plugin) |
941 | - plugin.run() |
942 | - |
943 | - message = plugin.create_mount_info_message() |
944 | - self.assertEqual(message, None) |
945 | - |
946 | - def test_ignore_removable_devices(self): |
947 | - """ |
948 | - The mount info plugin should only report data about |
949 | - non-removable devices. |
950 | - """ |
951 | - devices = [MockRealHALDevice({"info.udi": "wubble", |
952 | - "block.device": "/dev/scd0", |
953 | - "storage.removable": True})] |
954 | - filename = self.makeFile("""\ |
955 | -/dev/scd0 /media/Xerox_M750 iso9660 ro,nosuid,nodev,uid=1000,utf8 0 0 |
956 | -""") |
957 | - plugin = self.get_mount_info(mounts_file=filename, hal_devices=devices, |
958 | - mtab_file=filename) |
959 | - self.monitor.add(plugin) |
960 | - plugin.run() |
961 | - |
962 | - message = plugin.create_mount_info_message() |
963 | - self.assertEqual(message, None) |
964 | - |
965 | - def test_ignore_removable_devices_gudev(self): |
966 | - """ |
967 | - The mount info plugin uses gudev to retrieve removable information |
968 | - about devices. |
969 | - """ |
970 | - filename = self.makeFile("""\ |
971 | -/dev/scd0 /media/Xerox_M750 iso9660 ro,nosuid,nodev,uid=1000,utf8 0 0 |
972 | -""") |
973 | - plugin = self.get_mount_info(mounts_file=filename, |
974 | - mtab_file=filename) |
975 | - plugin._hal_manager = None |
976 | - |
977 | - class MockDevice(object): |
978 | - def get_sysfs_attr_as_boolean(self, attr): |
979 | - if attr == "removable": |
980 | - return True |
981 | - |
982 | - class MockGudevClient(object): |
983 | - def query_by_device_file(self, name): |
984 | - if name == "/dev/scd0": |
985 | - return MockDevice() |
986 | - |
987 | - plugin._gudev_client = MockGudevClient() |
988 | - self.monitor.add(plugin) |
989 | - plugin.run() |
990 | - |
991 | - message = plugin.create_mount_info_message() |
992 | - self.assertEqual(message, None) |
993 | - |
994 | - def test_ignore_multiparented_removable_devices(self): |
995 | - """ |
996 | - Some removable devices might be the grand-children of a device that is |
997 | - marked as "storage.removable". |
998 | - """ |
999 | - devices = [MockRealHALDevice({"info.udi": "wubble", |
1000 | - "block.device": "/dev/scd", |
1001 | - "storage.removable": True}), |
1002 | - MockRealHALDevice({"info.udi": "wubble0", |
1003 | - "block.device": "/dev/scd0", |
1004 | - "info.parent": "wubble"}), |
1005 | - MockRealHALDevice({"info.udi": "wubble0a", |
1006 | - "block.device": "/dev/scd0a", |
1007 | - "info.parent": "wubble0"}), |
1008 | - MockRealHALDevice({"info.udi": "wubble0b", |
1009 | - "block.device": "/dev/scd0b", |
1010 | - "info.parent": "wubble0"})] |
1011 | - |
1012 | - filename = self.makeFile("""\ |
1013 | -/dev/scd0a /media/Xerox_M750 iso9660 ro,nosuid,nodev,uid=1000,utf8 0 0 |
1014 | -""") |
1015 | - plugin = self.get_mount_info(mounts_file=filename, hal_devices=devices, |
1016 | - mtab_file=filename) |
1017 | - self.monitor.add(plugin) |
1018 | - plugin.run() |
1019 | - |
1020 | + "Removable" partitions are not reported to the server. |
1021 | + """ |
1022 | + filename = self.makeFile("""\ |
1023 | +/dev/hdc4 /mm xfs rw 0 0""") |
1024 | + plugin = self.get_mount_info(mounts_file=filename, mtab_file=filename) |
1025 | + |
1026 | + plugin.is_device_removable = lambda x: True # They are all removable |
1027 | + |
1028 | + self.monitor.add(plugin) |
1029 | + plugin.run() |
1030 | message = plugin.create_mount_info_message() |
1031 | self.assertEqual(message, None) |
1032 | |
1033 | @@ -549,6 +462,7 @@ |
1034 | """) |
1035 | plugin = MountInfo(mounts_file=filename, create_time=self.reactor.time, |
1036 | statvfs=statvfs, mtab_file=mtab_filename) |
1037 | + |
1038 | self.monitor.add(plugin) |
1039 | plugin.run() |
1040 | message = plugin.create_mount_info_message() |
1041 | @@ -622,6 +536,7 @@ |
1042 | /dev/hda2 /usr ext3 rw 0 0 |
1043 | /opt /mnt none rw,bind 0 0 |
1044 | """) |
1045 | + |
1046 | plugin = MountInfo(mounts_file=filename, create_time=self.reactor.time, |
1047 | statvfs=statvfs, mtab_file=mtab_filename) |
1048 | self.monitor.add(plugin) |
1049 | |
1050 | === removed file 'landscape/tests/test_hal.py' |
1051 | --- landscape/tests/test_hal.py 2012-03-06 09:17:51 +0000 |
1052 | +++ landscape/tests/test_hal.py 1970-01-01 00:00:00 +0000 |
1053 | @@ -1,87 +0,0 @@ |
1054 | -from dbus import SystemBus, Interface |
1055 | -from dbus.exceptions import DBusException |
1056 | - |
1057 | -from landscape.hal import HALDevice, HALManager |
1058 | -from landscape.tests.helpers import LandscapeTest |
1059 | - |
1060 | - |
1061 | -class HALManagerTest(LandscapeTest): |
1062 | - |
1063 | - def setUp(self): |
1064 | - super(HALManagerTest, self).setUp() |
1065 | - self.bus = SystemBus() |
1066 | - |
1067 | - def test_get_devices(self): |
1068 | - """ |
1069 | - A HALManager can return a flat list of devices. All available |
1070 | - devices should be included in the returned list. |
1071 | - """ |
1072 | - devices = HALManager().get_devices() |
1073 | - manager = self.bus.get_object("org.freedesktop.Hal", |
1074 | - "/org/freedesktop/Hal/Manager") |
1075 | - manager = Interface(manager, "org.freedesktop.Hal.Manager") |
1076 | - expected_devices = manager.GetAllDevices() |
1077 | - actual_devices = [device.udi for device in devices] |
1078 | - self.assertEqual(set(expected_devices), set(actual_devices)) |
1079 | - |
1080 | - def test_get_devices_with_dbus_error(self): |
1081 | - """ |
1082 | - If the L{HALManager} fails connecting to HAL over D-Bus, then the |
1083 | - L{HALManager.get_devices} method returns an empty list. |
1084 | - """ |
1085 | - self.log_helper.ignore_errors("Couldn't connect to Hal via DBus") |
1086 | - bus = self.mocker.mock() |
1087 | - bus.get_object("org.freedesktop.Hal", "/org/freedesktop/Hal/Manager") |
1088 | - self.mocker.throw(DBusException()) |
1089 | - self.mocker.replay() |
1090 | - devices = HALManager(bus=bus).get_devices() |
1091 | - self.assertEqual(devices, []) |
1092 | - |
1093 | - def test_get_devices_with_no_server(self): |
1094 | - """ |
1095 | - If the L{HALManager} fails connecting to HAL over D-Bus, for example |
1096 | - because the DBus server is not running at all, then the |
1097 | - L{HALManager.get_devices} method returns an empty list. |
1098 | - """ |
1099 | - self.log_helper.ignore_errors("Couldn't connect to Hal via DBus") |
1100 | - bus_mock = self.mocker.replace("dbus.SystemBus") |
1101 | - bus_mock() |
1102 | - self.mocker.throw(DBusException()) |
1103 | - self.mocker.replay() |
1104 | - devices = HALManager().get_devices() |
1105 | - self.assertEqual(devices, []) |
1106 | - |
1107 | - |
1108 | -class MockHALManager(object): |
1109 | - |
1110 | - def __init__(self, devices): |
1111 | - self.devices = devices |
1112 | - |
1113 | - def get_devices(self): |
1114 | - return [HALDevice(device) for device in self.devices] |
1115 | - |
1116 | - |
1117 | -class MockRealHALDevice(object): |
1118 | - |
1119 | - def __init__(self, properties): |
1120 | - self._properties = properties |
1121 | - self.udi = properties.get("info.udi", "fake_udi") |
1122 | - |
1123 | - def GetAllProperties(self): |
1124 | - return self._properties |
1125 | - |
1126 | - |
1127 | -class HALDeviceTest(LandscapeTest): |
1128 | - |
1129 | - def test_init(self): |
1130 | - device = HALDevice(MockRealHALDevice({"info.udi": "wubble"})) |
1131 | - self.assertEqual(device.properties, {"info.udi": "wubble"}) |
1132 | - self.assertEqual(device.udi, "wubble") |
1133 | - self.assertEqual(device.parent, None) |
1134 | - |
1135 | - def test_add_child(self): |
1136 | - parent = HALDevice(MockRealHALDevice({"info.udi": "wubble"})) |
1137 | - child = HALDevice(MockRealHALDevice({"info.udi": "ooga"})) |
1138 | - parent.add_child(child) |
1139 | - self.assertEqual(parent.get_children(), [child]) |
1140 | - self.assertEqual(child.parent, parent) |
Great idea to look at the file system, marking as needs fixing because of the tests. Will give a second round later.
[1]
+ """
+ A small utility to get a device's removable file path.
+ """
This can fit in one line. Please also document the device parameter, e.g.:
"""Get a device's removable file path.
This function figures out the C{/sys/ block/< device> /removable} path associated
with the given device. Such path indicates whether the device is removable
or not.
@param device: File system path of the device.
"""
[2]
+def get_device_ removable_ file_path( device) :
I think you can make this function private, since it's just an helper for is_device_removable and nothing outside this file is calling it (you can still test it by renaming the tests to "test_wb_xxx" to indicate they're whitebox test exercising private APIs).
Also please put the body of is_device_removable before the one of get_device_ removable_ file, since that's what readers will want to look at first.
[3]
+ if not device: # Shortcut the trivial case
+ return None
Can you elaborate on which cases would need to "device" being None or empty? It feels like the caller of this code should take care of that.
[4]
+def is_device_ removable( device, path=None):
Under which circumstances would path be None? And why is it an optional argument? (since the function doesn't seem to do anything interesting if you don't pass it).
[5]
I get these test failures (didn't really investigate):
[FAIL] python2. 7/dist- packages/ twisted/ internet/ defer.py" , line 138, in maybeDeferred python2. 7/dist- packages/ twisted/ internet/ _utilspy3. py", line 41, in runWithWarnings Suppressed exc_info[ 1], exc_info[2]) python2. 7/dist- packages/ twisted/ internet/ _utilspy3. py", line 37, in runWithWarnings Suppressed free/src/ client/ trunk/landscape /tests/ mocker. py", line 146, in test_method_wrapper free/src/ client/ trunk/landscape /monitor/ tests/test_ mountinfo. py", line 112, in test_read_ sample_ data assertEqual( len(mount_ info), 3) python2. 7/dist- packages/ twisted/ trial/_ synctest. py", line 356, in assertEqual trial.unittest. FailTest: not equal:
Traceback (most recent call last):
File "/usr/lib/
result = f(*args, **kw)
File "/usr/lib/
reraise(
File "/usr/lib/
result = f(*a, **kw)
File "/home/
result = test_method()
File "/home/
self.
File "/usr/lib/
% (msg, pformat(first), pformat(second)))
twisted.
a = 2
b = 3
landscape. monitor. tests.test_ mountinfo. MountInfoTest. test_read_ sample_ data ======= ======= ======= ======= ======= ======= ======= ======= ======= ======= == python2. 7/dist- packages/ twisted/ internet/ defer.py" , line 138, in maybeDeferred python2. 7/dist- packages/ twisted/ internet/ _utilspy3. py", line 41, in runWithWarnings Suppressed exc_info[ 1], exc_info[2]) python2. 7/dist- packages/ twisted/ internet/ _utilspy3. py", line 37, in runWithWarnings Suppressed free/src/ client/ trunk/landscape /tests/ mocker. py", line 146, in test_method_wrapper free/src/ cli...
=======
[ERROR]
Traceback (most recent call last):
File "/usr/lib/
result = f(*args, **kw)
File "/usr/lib/
reraise(
File "/usr/lib/
result = f(*a, **kw)
File "/home/
result = test_method()
File "/home/