Merge lp:~brendan-donegan/checkbox/bug960087_memory_compare_ram_devices into lp:checkbox

Proposed by Brendan Donegan
Status: Merged
Merged at revision: 1482
Proposed branch: lp:~brendan-donegan/checkbox/bug960087_memory_compare_ram_devices
Merge into: lp:checkbox
Diff against target: 211 lines (+103/-32) (has conflicts)
5 files modified
checkbox/lib/dmi.py (+16/-0)
checkbox/parsers/dmidecode.py (+4/-1)
debian/changelog (+9/-0)
scripts/dmi_resource (+4/-4)
scripts/memory_compare (+70/-27)
Text conflict in debian/changelog
To merge this branch: bzr merge lp:~brendan-donegan/checkbox/bug960087_memory_compare_ram_devices
Reviewer Review Type Date Requested Status
Marc Tardif (community) Approve
Brendan Donegan (community) Needs Resubmitting
Review via email: mp+112987@code.launchpad.net

Description of the change

I reimplemented and restructured the memory_compare script to use Python3 and also to store the dmi and meminfo output into dictionaries, which will allow for more flexibility later on if other fields need to be used to calculate the correct figures. This merge will also fix the bug where ROM devices for example are being included in the calculations, whereas we're only interested in RAM.

To post a comment you must log in.
1478. By Brendan Donegan

Merged from trunk

Revision history for this message
Marc Tardif (cr3) wrote :

Since the script is now written in Python, it should use the parsers in Checkbox to avoid duplicating that potentially error prone logic. Please modify the script to use checkbox.parsers.meminfo and checkbox.parsers.dmidecode. Thanks!

review: Needs Fixing
1479. By Brendan Donegan

Updated memory_compare to use meminfo parser from Checkbox instead of doing the parsing itself.

Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

I've updated it to use the meminfo parser. As for the dmi parser, I agree with the fundamental principle of code re-use here, but I'm not too happy that every time I want to use a new attribute I have three files that need to be updated (the parser, the DmiDevice object and the script). I've also had to modify the dmi_resource script to prevent it crapping out because of the new attributes being used (which aren't universal so some checking had to be added). I'll update with the changes that need to be done to use the parser though and we can take it from there.

1480. By Brendan Donegan

Refactored memory_compare python version to use DmidecodeParser

Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

I've made the necessary change to dmidecode.py and dmi.py to support the two extra attributes I need. I also extended dmi_resource.py to include them - let me know if you'd rather this wasn't included since it's a little outside the scope of this merge.

review: Needs Resubmitting
Revision history for this message
Marc Tardif (cr3) wrote :

In checkbox.lib.dmi, you added:

+ @property
+ def size(self):
+ attribute = "%s_size" % self.category.lower()
+ size = self._attributes.get(attribute)
+
+ size = int(size.split()[0]) * 1024
+
+ if size:
+ return size
+
+ return None

First, when the attribute exists, it will always interpret the size in kilobytes since it ignores the type in the split. Instead, you should use the string_to_type function defined in checkbox.lib.conversion.

Second, when the attribute doesn't exist, the split() method will fail horribly on a None value. So, I would suggest you write the size property method like this, remembering to import string_to_type at the beginning of the module of course:

+ @property
+ def size(self):
+ attribute = "%s_size" % self.category.lower()
+ size = self._attributes.get(attribute)
+ if size:
+ size = string_to_type(size)
+
+ return size

Still in checkbox.lib.dmi, you added:

+ @property
+ def form(self):
+ attribute = "%s_form" % self.category.lower()
+ form = self._attributes.get(attribute)
+
+ if form:
+ return form
+
+ return None

This could be expressed more simply as:

+ @property
+ def form(self):
+ attribute = "%s_form" % self.category.lower()
+ return self._attributes.get(attribute)

Other than that, the changes look good. After you make these changes, I'll have another look but I think we'll be good to merge. Thanks!

review: Needs Fixing
1481. By Brendan Donegan

Implemented cr3's suggestions.

Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

Okay, thanks for pointing out the string_to_type function - very useful! I've implemented these suggestions pretty much verbatim, so I guess we're done here!

review: Needs Resubmitting
Revision history for this message
Marc Tardif (cr3) wrote :

Nice, thanks for improving checkbox.parsers!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'checkbox/lib/dmi.py'
--- checkbox/lib/dmi.py 2011-10-11 20:15:01 +0000
+++ checkbox/lib/dmi.py 2012-07-03 18:18:18 +0000
@@ -18,6 +18,7 @@
18#18#
19import os19import os
2020
21from checkbox.lib.conversion import string_to_type
2122
22# See also 3.3.4.1 of the "System Management BIOS Reference Specification,23# See also 3.3.4.1 of the "System Management BIOS Reference Specification,
23# Version 2.6.1" (Preliminary Standard) document, available from24# Version 2.6.1" (Preliminary Standard) document, available from
@@ -223,3 +224,18 @@
223 return version224 return version
224225
225 return None226 return None
227
228 @property
229 def size(self):
230 attribute = "%s_size" % self.category.lower()
231 size = self._attributes.get(attribute)
232
233 if size:
234 size = string_to_type(size)
235
236 return size
237
238 @property
239 def form(self):
240 attribute = "%s_form" % self.category.lower()
241 return self._attributes.get(attribute)
226242
=== modified file 'checkbox/parsers/dmidecode.py'
--- checkbox/parsers/dmidecode.py 2012-06-01 13:57:25 +0000
+++ checkbox/parsers/dmidecode.py 2012-07-03 18:18:18 +0000
@@ -50,6 +50,8 @@
50 "Type": "type",50 "Type": "type",
51 "Vendor": "vendor",51 "Vendor": "vendor",
52 "Version": "version",52 "Version": "version",
53 "Size": "size",
54 "Form Factor": "form",
53 }55 }
5456
55 def __init__(self, stream):57 def __init__(self, stream):
@@ -97,11 +99,12 @@
97 category = Dmi.type_names[type_index]99 category = Dmi.type_names[type_index]
98 category = category.upper().split(" ")[-1]100 category = category.upper().split(" ")[-1]
99 if category not in (101 if category not in (
100 "BOARD", "BIOS", "CHASSIS", "PROCESSOR", "SYSTEM"):102 "BOARD", "BIOS", "CHASSIS", "DEVICE", "PROCESSOR", "SYSTEM"):
101 continue103 continue
102104
103 # Parse attributes105 # Parse attributes
104 attributes = {}106 attributes = {}
107
105 for line in lines:108 for line in lines:
106 # Skip lines with an unsupported key/value pair109 # Skip lines with an unsupported key/value pair
107 match = KEY_VALUE_RE.match(line)110 match = KEY_VALUE_RE.match(line)
108111
=== modified file 'debian/changelog'
--- debian/changelog 2012-07-03 15:19:26 +0000
+++ debian/changelog 2012-07-03 18:18:18 +0000
@@ -18,7 +18,16 @@
18 * Added a message file format test that does some simplistic checks18 * Added a message file format test that does some simplistic checks
19 on jobs/* files to ensure they are sane.19 on jobs/* files to ensure they are sane.
2020
21<<<<<<< TREE
21 -- Jeff Lane <jeff@ubuntu.com> Fri, 29 Jun 2012 16:30:33 -040022 -- Jeff Lane <jeff@ubuntu.com> Fri, 29 Jun 2012 16:30:33 -0400
23=======
24 [Brendan Donegan]
25 * Reimplemented memory_compare in python3 and restructured it to put
26 things into dictionaries for easy access. Also fixed bug with detecting
27 non-RAM devices as RAM. (LP: #960087)
28
29 -- Jeff Lane <jeff@ubuntu.com> Fri, 29 Jun 2012 15:46:37 -0400
30>>>>>>> MERGE-SOURCE
2231
23checkbox (0.14.1) quantal; urgency=low32checkbox (0.14.1) quantal; urgency=low
2433
2534
=== modified file 'scripts/dmi_resource'
--- scripts/dmi_resource 2012-05-31 20:21:08 +0000
+++ scripts/dmi_resource 2012-07-03 18:18:18 +0000
@@ -30,11 +30,11 @@
30class DmiResult:30class DmiResult:
3131
32 attributes = ("path", "category", "product", "vendor", "serial",32 attributes = ("path", "category", "product", "vendor", "serial",
33 "version",)33 "version", "size", "form",)
3434
35 def addDmiDevice(self, device):35 def addDmiDevice(self, device):
36 for attribute in self.attributes:36 for attribute in self.attributes:
37 value = getattr(device, attribute)37 value = getattr(device, attribute, None)
38 if value is not None:38 if value is not None:
39 print("%s: %s" % (attribute, value))39 print("%s: %s" % (attribute, value))
4040
@@ -43,10 +43,10 @@
4343
44def main():44def main():
45 stream = os.popen(COMMAND)45 stream = os.popen(COMMAND)
46 udev = DmidecodeParser(stream)46 dmi = DmidecodeParser(stream)
4747
48 result = DmiResult()48 result = DmiResult()
49 udev.run(result)49 dmi.run(result)
5050
51 return 051 return 0
5252
5353
=== modified file 'scripts/memory_compare'
--- scripts/memory_compare 2012-02-02 17:24:06 +0000
+++ scripts/memory_compare 2012-07-03 18:18:18 +0000
@@ -1,27 +1,70 @@
1#!/bin/bash1#!/usr/bin/env python3
22
3MIN_LEVEL=903import os
44import sys
5meminfo_total=`grep 'MemTotal' /proc/meminfo | awk '{print $2}'`5
6meminfo_units=`grep 'MemTotal' /proc/meminfo | awk '{print $3}'`6from checkbox.parsers.dmidecode import DmidecodeParser
77from checkbox.parsers.meminfo import MeminfoParser
8echo "Meminfo total: $meminfo_total $meminfo_units"8
99THRESHOLD = 10
10dmi_total=010
1111class DmiResult:
12for size in `dmidecode -t 17 | grep Size | grep -o [0-9]*`12
13do13 total_size = 0
14 dmi_total=`echo $dmi_total + $size | bc`14 form = None
15done15
1616 def addDmiDevice(self, device):
17dmi_total=`echo "$dmi_total * 1000" | bc`17 size = getattr(device, "size", None)
1818 form = getattr(device, "form", None)
19echo "DMI total: $dmi_total $meminfo_units"19
20accuracy=`echo "scale=2; $meminfo_total / $dmi_total * 100" | bc`20 if form and "IMM" in form:
21echo "Accuracy: $accuracy"21 self.form = form
2222
23if [ ${accuracy::-3} -lt $MIN_LEVEL ]23 if size:
24then24 self.total_size += size
25 echo "Memory totals not close enough"25
26 exit 126def get_installed_memory_size():
27fi27 dmi = DmidecodeParser(os.popen('dmidecode'))
28 result = DmiResult()
29
30 dmi.run(result)
31
32 return result.total_size
33
34class MeminfoResult:
35
36 memtotal = 0
37
38 def setMemory(self, memory):
39 self.memtotal = memory['total']
40
41def get_visible_memory_size():
42 parser = MeminfoParser(open('/proc/meminfo'))
43 result = MeminfoResult()
44 parser.run(result)
45
46 return result.memtotal
47
48def main():
49 if os.geteuid() != 0:
50 print("This script must be run as root.")
51 return 1
52
53 installed_memory = get_installed_memory_size()
54 print("Installed memory: %d kB" % installed_memory)
55
56 visible_memory = get_visible_memory_size()
57 print("Visible memory: %d kB" % visible_memory)
58
59 difference = installed_memory - visible_memory
60 percentage = difference / installed_memory * 100
61
62 if percentage <= THRESHOLD:
63 print("Difference is %d bytes (%.2f%%) and less than the %d%% threshold." % (difference, percentage, THRESHOLD))
64 return 0
65 else:
66 print("Difference is %d bytes (%.2f%%) and greater than the %d%% threshold." % (difference, percentage, THRESHOLD))
67 return 1
68
69if __name__ == "__main__":
70 sys.exit(main())

Subscribers

People subscribed via source and target branches