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

[Brendan Donegan]

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.

[lp:~brendan-donegan/checkbox/bug960087_memory_compare_ram_devices]

1478. By Brendan Donegan on 2012-07-02

Merged from trunk

[Marc Tardif (cr3)]

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

[lp:~brendan-donegan/checkbox/bug960087_memory_compare_ram_devices]

1479. By Brendan Donegan on 2012-07-02

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

[Brendan Donegan (brendan-donegan)]

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.

[lp:~brendan-donegan/checkbox/bug960087_memory_compare_ram_devices]

1480. By Brendan Donegan on 2012-07-03

Refactored memory_compare python version to use DmidecodeParser

[Brendan Donegan (brendan-donegan)]

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

[Marc Tardif (cr3)]

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

[lp:~brendan-donegan/checkbox/bug960087_memory_compare_ram_devices]

1481. By Brendan Donegan on 2012-07-03

Implemented cr3's suggestions.

[Brendan Donegan (brendan-donegan)]

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

[Marc Tardif (cr3)]

Nice, thanks for improving checkbox.parsers!

review: Approve

=== 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 @@
 #
 import os
 
+from checkbox.lib.conversion import string_to_type
 
 # See also 3.3.4.1 of the "System Management BIOS Reference Specification,
 # Version 2.6.1" (Preliminary Standard) document, available from
@@ -223,3 +224,18 @@
             return version
 
         return None
+
+    @property
+    def size(self):
+        attribute = "%s_size" % self.category.lower()
+        size = self._attributes.get(attribute)
+
+        if size:
+            size = string_to_type(size)
+
+        return size
+
+    @property
+    def form(self):
+        attribute = "%s_form" % self.category.lower()
+        return self._attributes.get(attribute)

=== 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 @@
         "Type": "type",
         "Vendor": "vendor",
         "Version": "version",
+        "Size": "size",
+        "Form Factor": "form",
         }
 
     def __init__(self, stream):
@@ -97,11 +99,12 @@
             category = Dmi.type_names[type_index]
             category = category.upper().split(" ")[-1]
             if category not in (
-                "BOARD", "BIOS", "CHASSIS", "PROCESSOR", "SYSTEM"):
+                "BOARD", "BIOS", "CHASSIS", "DEVICE", "PROCESSOR", "SYSTEM"):
                 continue
 
             # Parse attributes
             attributes = {}
+            
             for line in lines:
                 # Skip lines with an unsupported key/value pair
                 match = KEY_VALUE_RE.match(line)

=== 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 @@
   * Added a message file format test that does some simplistic checks
     on jobs/* files to ensure they are sane.
 
+<<<<<<< TREE
  -- Jeff Lane <[e-mail]>  Fri, 29 Jun 2012 16:30:33 -0400
+=======
+  [Brendan Donegan]
+  * Reimplemented memory_compare in python3 and restructured it to put
+    things into dictionaries for easy access. Also fixed bug with detecting
+    non-RAM devices as RAM. (LP: #960087)
+
+ -- Jeff Lane <[e-mail]>  Fri, 29 Jun 2012 15:46:37 -0400
+>>>>>>> MERGE-SOURCE
 
 checkbox (0.14.1) quantal; urgency=low
 

=== 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 @@
 class DmiResult:
 
     attributes = ("path", "category", "product", "vendor", "serial",
-        "version",)
+        "version", "size", "form",)
 
     def addDmiDevice(self, device):
         for attribute in self.attributes:
-            value = getattr(device, attribute)
+            value = getattr(device, attribute, None)
             if value is not None:
                 print("%s: %s" % (attribute, value))
 
@@ -43,10 +43,10 @@
 
 def main():
     stream = os.popen(COMMAND)
-    udev = DmidecodeParser(stream)
+    dmi = DmidecodeParser(stream)
 
     result = DmiResult()
-    udev.run(result)
+    dmi.run(result)
 
     return 0
 

=== 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 @@
-#!/bin/bash
-
-MIN_LEVEL=90
-
-meminfo_total=`grep 'MemTotal' /proc/meminfo | awk '{print $2}'`
-meminfo_units=`grep 'MemTotal' /proc/meminfo | awk '{print $3}'`
-
-echo "Meminfo total: $meminfo_total $meminfo_units"
-
-dmi_total=0
-
-for size in `dmidecode -t 17 | grep Size | grep -o [0-9]*`
-do
-    dmi_total=`echo $dmi_total + $size | bc`
-done
-
-dmi_total=`echo "$dmi_total * 1000" | bc`
-
-echo "DMI total: $dmi_total $meminfo_units"
-accuracy=`echo "scale=2; $meminfo_total / $dmi_total * 100" | bc`
-echo "Accuracy: $accuracy"
-
-if [ ${accuracy::-3} -lt $MIN_LEVEL ]
-then
-    echo "Memory totals not close enough"
-    exit 1
-fi
+#!/usr/bin/env python3
+
+import os
+import sys
+
+from checkbox.parsers.dmidecode import DmidecodeParser
+from checkbox.parsers.meminfo import MeminfoParser
+
+THRESHOLD = 10
+
+class DmiResult:
+
+    total_size = 0
+    form = None
+
+    def addDmiDevice(self, device):
+        size = getattr(device, "size", None)
+        form = getattr(device, "form", None)
+
+        if form and "IMM" in form:
+            self.form = form
+        
+        if size:
+            self.total_size += size
+
+def get_installed_memory_size():
+    dmi = DmidecodeParser(os.popen('dmidecode'))
+    result = DmiResult()
+
+    dmi.run(result)
+
+    return result.total_size
+
+class MeminfoResult:
+
+    memtotal = 0
+
+    def setMemory(self, memory):
+        self.memtotal = memory['total']
+
+def get_visible_memory_size():
+    parser = MeminfoParser(open('/proc/meminfo'))
+    result = MeminfoResult()
+    parser.run(result)
+
+    return result.memtotal
+
+def main():
+    if os.geteuid() != 0:
+        print("This script must be run as root.")
+        return 1
+
+    installed_memory = get_installed_memory_size()
+    print("Installed memory: %d kB" % installed_memory)
+
+    visible_memory = get_visible_memory_size()
+    print("Visible memory: %d kB" % visible_memory)
+
+    difference = installed_memory - visible_memory
+    percentage = difference / installed_memory * 100
+
+    if percentage <= THRESHOLD:
+        print("Difference is %d bytes (%.2f%%) and less than the %d%% threshold." % (difference, percentage, THRESHOLD))
+        return 0
+    else:
+        print("Difference is %d bytes (%.2f%%) and greater than the %d%% threshold." % (difference, percentage, THRESHOLD))
+        return 1
+
+if __name__ == "__main__":
+    sys.exit(main())