[Qemu-devel] [PATCH v3 0/3] Vmstate-static-checker.py fix upstream

[Qemu-devel] [PATCH v3 0/3] Vmstate-static-checker.py fix upstream

[dverma]

This is an update to the script vmstate-static-checker.py. The whitelist has 
been updated and newer functions have been added to reduce the false
positives generated by the script while testing migration. The code has been
cleaned and updated to follow PEP8 guidelines.

dverma (3):
  Fix format and styles; make code more pythonic
  Update the existing whitelist
  Add new functions for whitelisting and their calls

 scripts/vmstate-static-checker.py | 335 ++++++++++++++++++++++++++++----------
 1 file changed, 250 insertions(+), 85 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/3] Fix format and styles; make code more pythonic

[dverma]

- Format fixes, cleaned up the print statement
- Style fixes, e.g. changed "if not x in y" to "if x not in y"
- Improved variable names
---
 scripts/vmstate-static-checker.py | 111 +++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 49 deletions(-)

diff --git a/scripts/vmstate-static-checker.py b/scripts/vmstate-static-checker.py
index bcef7ee..b416b66 100755
--- a/scripts/vmstate-static-checker.py
+++ b/scripts/vmstate-static-checker.py
@@ -19,6 +19,11 @@
 # You should have received a copy of the GNU General Public License along
 # with this program; if not, see <http://www.gnu.org/licenses/>.
 
+#
+# 2017 Deepak Verma <dverma@redhat.com>
+# Added few functions and fields for whitelisting
+#
+
 import argparse
 import json
 import sys
@@ -26,6 +31,7 @@ import sys
 # Count the number of errors found
 taint = 0
 
+
 def bump_taint():
     global taint
 
@@ -92,7 +98,7 @@ def check_fields_match(name, s_field, d_field):
                       'io_win_size', 'mig_io_win_size'],
     }
 
-    if not name in changed_names:
+    if name not in changed_names:
         return False
 
     if s_field in changed_names[name] and d_field in changed_names[name]:
@@ -100,6 +106,7 @@ def check_fields_match(name, s_field, d_field):
 
     return False
 
+
 def get_changed_sec_name(sec):
     # Section names can change -- see commit 292b1634 for an example.
     changes = {
@@ -114,16 +121,17 @@ def get_changed_sec_name(sec):
             return item
     return ""
 
+
 def exists_in_substruct(fields, item):
     # Some QEMU versions moved a few fields inside a substruct.  This
     # kept the on-wire format the same.  This function checks if
     # something got shifted inside a substruct.  For example, the
     # change in commit 1f42d22233b4f3d1a2933ff30e8d6a6d9ee2d08f
 
-    if not "Description" in fields:
+    if "Description" not in fields:
         return False
 
-    if not "Fields" in fields["Description"]:
+    if "Fields" not in fields["Description"]:
         return False
 
     substruct_fields = fields["Description"]["Fields"]
@@ -176,10 +184,10 @@ def check_fields(src_fields, dest_fields, desc, sec):
             except StopIteration:
                 if d_iter_list == []:
                     # We were not in a substruct
-                    print "Section \"" + sec + "\",",
-                    print "Description " + "\"" + desc + "\":",
-                    print "expected field \"" + s_item["field"] + "\",",
-                    print "while dest has no further fields"
+                    print('Section "' + sec + '", '
+                          'Description "' + desc + '": '
+                          'expected field "' + s_item["field"] + '", '
+                          'while dest has no further fields')
                     bump_taint()
                     break
 
@@ -191,30 +199,28 @@ def check_fields(src_fields, dest_fields, desc, sec):
                 advance_dest = True
 
         if unused_count != 0:
-            if advance_dest == False:
+            if not advance_dest:
                 unused_count = unused_count - s_item["size"]
                 if unused_count == 0:
                     advance_dest = True
                     continue
                 if unused_count < 0:
-                    print "Section \"" + sec + "\",",
-                    print "Description \"" + desc + "\":",
-                    print "unused size mismatch near \"",
-                    print s_item["field"] + "\""
+                    print('Section "' + sec + '", '
+                          'Description "' + desc + '": '
+                          'unused size mismatch near "' + s_item["field"] + '"')
                     bump_taint()
                     break
                 continue
 
-            if advance_src == False:
+            if not advance_src:
                 unused_count = unused_count - d_item["size"]
                 if unused_count == 0:
                     advance_src = True
                     continue
                 if unused_count < 0:
-                    print "Section \"" + sec + "\",",
-                    print "Description \"" + desc + "\":",
-                    print "unused size mismatch near \"",
-                    print d_item["field"] + "\""
+                    print('Section "' + sec + '", '
+                          'Description "' + desc + '": '
+                          'unused size mismatch near "' + d_item["field"] + '"')
                     bump_taint()
                     break
                 continue
@@ -262,16 +268,16 @@ def check_fields(src_fields, dest_fields, desc, sec):
                     unused_count = s_item["size"] - d_item["size"]
                     continue
 
-            print "Section \"" + sec + "\",",
-            print "Description \"" + desc + "\":",
-            print "expected field \"" + s_item["field"] + "\",",
-            print "got \"" + d_item["field"] + "\"; skipping rest"
+            print('Section "' + sec + '", '
+                  'Description "' + desc + '": '
+                  'expected field "' + s_item["field"] + '", '
+                  'got "' + d_item["field"] + '"; skipping rest')
             bump_taint()
             break
 
         check_version(s_item, d_item, sec, desc)
 
-        if not "Description" in s_item:
+        if "Description" not in s_item:
             # Check size of this field only if it's not a VMSTRUCT entry
             check_size(s_item, d_item, sec, desc, s_item["field"])
 
@@ -289,18 +295,20 @@ def check_subsections(src_sub, dest_sub, desc, sec):
             check_descriptions(s_item, d_item, sec)
 
         if not found:
-            print "Section \"" + sec + "\", Description \"" + desc + "\":",
-            print "Subsection \"" + s_item["name"] + "\" not found"
+            print('Section "' + sec + '", '
+                  'Description "' + desc + '": '
+                  'Subsection "' + s_item["name"] + '" not found')
             bump_taint()
 
 
 def check_description_in_list(s_item, d_item, sec, desc):
-    if not "Description" in s_item:
+    if "Description" not in s_item:
         return
 
-    if not "Description" in d_item:
-        print "Section \"" + sec + "\", Description \"" + desc + "\",",
-        print "Field \"" + s_item["field"] + "\": missing description"
+    if "Description" not in d_item:
+        print('Section "' + sec + '", '
+              'Description "' + desc + '", '
+              'Field "' + s_item["field"] + '": missing description')
         bump_taint()
         return
 
@@ -311,17 +319,17 @@ def check_descriptions(src_desc, dest_desc, sec):
     check_version(src_desc, dest_desc, sec, src_desc["name"])
 
     if not check_fields_match(sec, src_desc["name"], dest_desc["name"]):
-        print "Section \"" + sec + "\":",
-        print "Description \"" + src_desc["name"] + "\"",
-        print "missing, got \"" + dest_desc["name"] + "\" instead; skipping"
+        print('Section "' + sec + '": '
+              'Description "' + src_desc["name"] + '" '
+              'missing, got "' + dest_desc["name"] + '" instead; skipping')
         bump_taint()
         return
 
     for f in src_desc:
         if not f in dest_desc:
-            print "Section \"" + sec + "\"",
-            print "Description \"" + src_desc["name"] + "\":",
-            print "Entry \"" + f + "\" missing"
+            print('Section "' + sec + '" '
+                  'Description "' + src_desc["name"] + '": '
+                  'Entry "' + field + '" missing')
             bump_taint()
             continue
 
@@ -340,15 +348,15 @@ def check_version(s, d, sec, desc=None):
         print "version error:", s["version_id"], ">", d["version_id"]
         bump_taint()
 
-    if not "minimum_version_id" in d:
+    if "minimum_version_id" not in dest_ver:
         return
 
     if s["version_id"] < d["minimum_version_id"]:
         print "Section \"" + sec + "\"",
         if desc:
-            print "Description \"" + desc + "\":",
-            print "minimum version error:", s["version_id"], "<",
-            print d["minimum_version_id"]
+            print('Description "' + desc + '": ' +
+                  'minimum version error: ' + str(src_ver["version_id"]) + ' < ' +
+                  str(dest_ver["minimum_version_id"]))
             bump_taint()
 
 
@@ -363,15 +371,21 @@ def check_size(s, d, sec, desc=None, field=None):
         bump_taint()
 
 
-def check_machine_type(s, d):
-    if s["Name"] != d["Name"]:
-        print "Warning: checking incompatible machine types:",
-        print "\"" + s["Name"] + "\", \"" + d["Name"] + "\""
+
+def check_machine_type(src, dest):
+    if src["Name"] != dest["Name"]:
+        print('Warning: checking incompatible machine types: '
+              '"' + src["Name"] + '", "' + dest["Name"] + '"')
     return
 
 
 def main():
-    help_text = "Parse JSON-formatted vmstate dumps from QEMU in files SRC and DEST.  Checks whether migration from SRC to DEST QEMU versions would break based on the VMSTATE information contained within the JSON outputs.  The JSON output is created from a QEMU invocation with the -dump-vmstate parameter and a filename argument to it.  Other parameters to QEMU do not matter, except the -M (machine type) parameter."
+    help_text = ("Parse JSON-formatted vmstate dumps from QEMU in files "
+    "SRC and DEST.  Checks whether migration from SRC to DEST QEMU versions "
+    "would break based on the VMSTATE information contained within the JSON "
+    "outputs. The JSON output is created from a QEMU invocation with the "
+    "-dump-vmstate parameter and a filename argument to it. Other parameters to "
+    "QEMU do not matter, except the -M (machine type) parameter.")
 
     parser = argparse.ArgumentParser(description=help_text)
     parser.add_argument('-s', '--src', type=file, required=True,
@@ -395,12 +409,12 @@ def main():
 
     for sec in src_data:
         dest_sec = sec
-        if not dest_sec in dest_data:
+        if dest_sec not in dest_data:
             # Either the section name got changed, or the section
             # doesn't exist in dest.
             dest_sec = get_changed_sec_name(sec)
-            if not dest_sec in dest_data:
-                print "Section \"" + sec + "\" does not exist in dest"
+            if dest_sec not in dest_data:
+                print('Section "' + sec + '" does not exist in dest')
                 bump_taint()
                 continue
 
@@ -414,9 +428,8 @@ def main():
         check_version(s, d, sec)
 
         for entry in s:
-            if not entry in d:
-                print "Section \"" + sec + "\": Entry \"" + entry + "\"",
-                print "missing"
+            if entry not in d:
+                print('Section "' + sec + '": Entry "' + entry + '" missing')
                 bump_taint()
                 continue
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/3] Update the existing whitelist

[dverma]

Appended newer fields and introduced new names in the whitelist
---
 scripts/vmstate-static-checker.py | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/scripts/vmstate-static-checker.py b/scripts/vmstate-static-checker.py
index b416b66..ae41e44 100755
--- a/scripts/vmstate-static-checker.py
+++ b/scripts/vmstate-static-checker.py
@@ -49,7 +49,6 @@ def check_fields_match(name, s_field, d_field):
     # is used to whitelist such changes in each section / description.
     changed_names = {
         'apic': ['timer', 'timer_expiry'],
-        'e1000': ['dev', 'parent_obj'],
         'ehci': ['dev', 'pcidev'],
         'I440FX': ['dev', 'parent_obj'],
         'ich9_ahci': ['card', 'parent_obj'],
@@ -73,7 +72,6 @@ def check_fields_match(name, s_field, d_field):
                      'tmr.timer', 'ar.tmr.timer',
                      'tmr.overflow_time', 'ar.tmr.overflow_time',
                      'gpe', 'ar.gpe'],
-        'rtl8139': ['dev', 'parent_obj'],
         'qxl': ['num_surfaces', 'ssd.num_surfaces'],
         'usb-ccid': ['abProtocolDataStructure', 'abProtocolDataStructure.data'],
         'usb-host': ['dev', 'parent_obj'],
@@ -96,6 +94,26 @@ def check_fields_match(name, s_field, d_field):
                       'mem_win_size', 'mig_mem_win_size',
                       'io_win_addr', 'mig_io_win_addr',
                       'io_win_size', 'mig_io_win_size'],
+        'rtl8139': ['dev', 'parent_obj'],
+        'e1000e': ['PCIDevice', 'PCIEDevice', 'intr_state', 'redhat_7_3_intr_state'],
+        'nec-usb-xhci': ['PCIDevice', 'PCIEDevice'],
+        'xhci-intr': ['er_full_unused', 'er_full'],
+        'e1000': ['dev', 'parent_obj',
+                  'tx.ipcss', 'tx.props.ipcss',
+                  'tx.ipcso', 'tx.props.ipcso',
+                  'tx.ipcse', 'tx.props.ipcse',
+                  'tx.tucss', 'tx.props.tucss',
+                  'tx.tucso', 'tx.props.tucso',
+                  'tx.tucse', 'tx.props.tucse',
+                  'tx.paylen', 'tx.props.paylen',
+                  'tx.hdr_len', 'tx.props.hdr_len',
+                  'tx.mss', 'tx.props.mss',
+                  'tx.sum_needed', 'tx.props.sum_needed',
+                  'tx.ip', 'tx.props.ip',
+                  'tx.tcp', 'tx.props.tcp',
+                  'tx.ipcss', 'tx.props.ipcss',
+                  'tx.ipcss', 'tx.props.ipcss',
+                  ]
     }
 
     if name not in changed_names:
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/3] Add new functions for whitelisting and their calls

[dverma]

The 'check_updated_properties' function keeps track of properties
that were added/removed from fields across qemu versions. The
'check_updated_sizes' function reduces false positives generated
especially while testing backward migration by keeping a list
of common size/version changes. The 'check_new_sections' function
is used to check for sections that got deprecated or were introduced
in different versions of qemu and will show as false positives while
testing forward migration. Improved the variable names and added
multiple blank newlines to keep Python PEP8 warning away.
---
 scripts/vmstate-static-checker.py | 226 ++++++++++++++++++++++++++++++--------
 1 file changed, 180 insertions(+), 46 deletions(-)

diff --git a/scripts/vmstate-static-checker.py b/scripts/vmstate-static-checker.py
index ae41e44..77e63f1 100755
--- a/scripts/vmstate-static-checker.py
+++ b/scripts/vmstate-static-checker.py
@@ -40,6 +40,107 @@ def bump_taint():
     if taint < 255:
         taint = taint + 1
 
+# Sections gain/lose new fields with time.
+# These are not name changes thats handled by another list.
+# These will be 'missing' or 'not found' in different versions of qemu
+
+
+def check_updated_properties(src_desc, field):
+    src_desc = str(src_desc)
+    field = str(field)
+    updated_property = {
+      'ICH9LPC': ['ICH9LPC/smi_feat'],
+      'ide_bus/error': ['retry_sector_num', 'retry_nsector', 'retry_unit'],
+      'e1000': ['e1000/full_mac_state'],
+      'ich9_pm': ['ich9_pm/tco', 'ich9_pm/cpuhp']
+    }
+
+    if src_desc in updated_property and field in updated_property[src_desc]:
+        return True
+
+    return False
+
+
+# A lot of errors are generated due to differences in sizes some of which are false positives. This list
+# is used to save those common changes
+def check_updated_sizes(field, old_size, new_size):
+    new_sizes_list = {
+            'tally_counters.TxOk': [8, 64],
+            'intel-iommu': [0, 1],
+            'iommu-intel': [0, 1]
+            }
+
+    if field not in new_sizes_list:
+        return False
+
+    if (old_size in new_sizes_list[field] and new_size in new_sizes_list[field]):
+        return True
+
+    return False
+
+
+# With time new sections/hardwares supported and old ones are depreciated on
+# chipsets.
+# There is no separate list for new or dead sections as it's relative to which
+# qemu version you compare too.
+# Update this list with such sections.
+# some items in this list might overlap with changed sections names.
+def check_new_sections(sec):
+    new_sections_list = [
+            'virtio-balloon-device',
+            'virtio-rng-device',
+            'virtio-scsi-device',
+            'virtio-blk-device',
+            'virtio-serial-device',
+            'virtio-net-device',
+            'vhost-vsock-device',
+            'virtio-input-host-device',
+            'virtio-input-hid-device',
+            'virtio-mouse-device',
+            'virtio-keyboard-device',
+            'virtio-vga',
+            'virtio-input-device',
+            'virtio-gpu-device',
+            'virtio-tablet-device',
+            'isa-pcspk',
+            'qemu-xhci',
+            'base-xhci',
+            'vmgenid',
+            'intel-iommu',
+            'i8257',
+            'i82801b11-bridge',
+            'ivshmem',
+            'ivshmem-doorbell',
+            'ivshmem-plain',
+            'usb-storage-device',
+            'usb-storage-dev',
+            'pci-qxl',
+            'pci-uhci-usb',
+            'pci-piix3',
+            'pci-vga',
+            'pci-bridge-seat',
+            'pcie-root-port',
+            'fw_cfg_io',
+            'fw_cfg_mem',
+            'exynos4210-ehci-usb',
+            'sysbus-ehci-usb',
+            'tegra2-ehci-usb',
+            'kvm-apic',
+            'fusbh200-ehci-usb',
+            'apic',
+            'apic-common',
+            'xlnx,ps7-usb',
+            'e1000e',
+            'e1000-82544gc',
+            'e1000-82545em']
+
+    if sec in new_sections_list:
+        return True
+
+    return False
+
+# Fields might change name with time across qemu versions.
+
 
 def check_fields_match(name, s_field, d_field):
     if s_field == d_field:
@@ -286,12 +387,22 @@ def check_fields(src_fields, dest_fields, desc, sec):
                     unused_count = s_item["size"] - d_item["size"]
                     continue
 
-            print('Section "' + sec + '", '
-                  'Description "' + desc + '": '
-                  'expected field "' + s_item["field"] + '", '
-                  'got "' + d_item["field"] + '"; skipping rest')
-            bump_taint()
-            break
+            # commit 20daa90a20d, extra field 'config' was added in newer releases
+            # there will be a mismatch in the number of fields of irq_state and config
+            # it's a known false positive so skip it
+            if (desc in ["PCIDevice", "PCIEDevice"]):
+                if((s_item["field"] in ["irq_state", "config"]) and
+                   (d_item["field"] in ["irq_state", "config"])):
+                    break
+
+            # some fields are new some dead, but are not errors.
+            if not check_fields_match(desc, s_item["field"], d_item["field"]):
+                print('Section "' + sec + '", '
+                      'Description "' + desc + '": '
+                      'expected field "' + s_item["field"] + '", '
+                      'got "' + d_item["field"] + '"; skipping rest')
+                bump_taint()
+                break
 
         check_version(s_item, d_item, sec, desc)
 
@@ -312,7 +423,8 @@ def check_subsections(src_sub, dest_sub, desc, sec):
             found = True
             check_descriptions(s_item, d_item, sec)
 
-        if not found:
+        # check the updated properties list before throwing error
+        if not found and (not check_updated_properties(desc, s_item["name"])):
             print('Section "' + sec + '", '
                   'Description "' + desc + '": '
                   'Subsection "' + s_item["name"] + '" not found')
@@ -343,51 +455,59 @@ def check_descriptions(src_desc, dest_desc, sec):
         bump_taint()
         return
 
-    for f in src_desc:
-        if not f in dest_desc:
-            print('Section "' + sec + '" '
-                  'Description "' + src_desc["name"] + '": '
-                  'Entry "' + field + '" missing')
-            bump_taint()
-            continue
+    for field in src_desc:
+        if field not in dest_desc:
+            # check the updated list of changed properties before throwing error
+            if check_updated_properties(src_desc["name"], field):
+                continue
+            else:
+                print('Section "' + sec + '" '
+                      'Description "' + src_desc["name"] + '": '
+                      'Entry "' + field + '" missing')
+                bump_taint()
+                continue
 
-        if f == 'Fields':
-            check_fields(src_desc[f], dest_desc[f], src_desc["name"], sec)
+        if field == 'Fields':
+            check_fields(src_desc[field], dest_desc[field], src_desc["name"], sec)
 
-        if f == 'Subsections':
-            check_subsections(src_desc[f], dest_desc[f], src_desc["name"], sec)
+        if field == 'Subsections':
+            check_subsections(src_desc[field], dest_desc[field], src_desc["name"], sec)
 
 
-def check_version(s, d, sec, desc=None):
-    if s["version_id"] > d["version_id"]:
-        print "Section \"" + sec + "\"",
-        if desc:
-            print "Description \"" + desc + "\":",
-        print "version error:", s["version_id"], ">", d["version_id"]
-        bump_taint()
+def check_version(src_ver, dest_ver, sec, desc=None):
+    if src_ver["version_id"] > dest_ver["version_id"]:
+        if not check_updated_sizes(sec, src_ver["version_id"], dest_ver["version_id"]):
+            print ('Section "' + sec + '"'),
+            if desc:
+                print ('Description "' + desc + '":'),
+            print ('version error: ' + str(src_ver["version_id"]) + ' > ' + str(dest_ver["version_id"]))
+            bump_taint()
 
     if "minimum_version_id" not in dest_ver:
         return
 
-    if s["version_id"] < d["minimum_version_id"]:
-        print "Section \"" + sec + "\"",
-        if desc:
-            print('Description "' + desc + '": ' +
-                  'minimum version error: ' + str(src_ver["version_id"]) + ' < ' +
-                  str(dest_ver["minimum_version_id"]))
-            bump_taint()
+    if src_ver["version_id"] < dest_ver["minimum_version_id"]:
+        if not check_updated_sizes(sec, src_ver["version_id"], dest_ver["minimum_version_id"]):
+            print ('Section "' + sec + '"'),
+            if desc:
+                print('Description "' + desc + '": ' +
+                      'minimum version error: ' + str(src_ver["version_id"]) + ' < ' +
+                      str(dest_ver["minimum_version_id"]))
+                bump_taint()
 
 
-def check_size(s, d, sec, desc=None, field=None):
-    if s["size"] != d["size"]:
-        print "Section \"" + sec + "\"",
-        if desc:
-            print "Description \"" + desc + "\"",
-        if field:
-            print "Field \"" + field + "\"",
-        print "size mismatch:", s["size"], ",", d["size"]
-        bump_taint()
+def check_size(src, dest, sec, desc=None, field=None):
+    if src["size"] != dest["size"]:
+        # check updated sizes list before throwing error
 
+        if not check_updated_sizes(field, src["size"], dest["size"]):
+            print ('Section "' + sec + '"'),
+            if desc:
+                print ('Description "' + desc + '"'),
+            if field:
+                print ('Field "' + field + '"'),
+            print ('size mismatch: ' + str(src["size"]) + ' , ' + str(dest["size"]))
+            bump_taint()
 
 
 def check_machine_type(src, dest):
@@ -428,13 +548,27 @@ def main():
     for sec in src_data:
         dest_sec = sec
         if dest_sec not in dest_data:
-            # Either the section name got changed, or the section
-            # doesn't exist in dest.
+            # Either the section name got changed, or
+            # the section doesn't exist in dest or
+            # section was newly supported.
             dest_sec = get_changed_sec_name(sec)
-            if dest_sec not in dest_data:
-                print('Section "' + sec + '" does not exist in dest')
-                bump_taint()
+            if dest_sec == "":
+                if not check_new_sections(sec):
+                    # section not in newly supported list and not in dest
+                    print('Section "' + sec + '" does not exist in dest')
+                    bump_taint()
                 continue
+            else:
+                # section name changed
+                if dest_sec not in dest_data:
+                    # new name not found in dest.
+                    if check_new_sections(dest_sec):
+                        continue
+                    else:
+                        # new name not in dest and not newly supported
+                        print('Section "' + sec + '" does not exist in dest')
+                        bump_taint()
+                        continue
 
         s = src_data[sec]
         d = dest_data[dest_sec]
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 1/3] Fix format and styles; make code more pythonic

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On 08/07/2017 11:53 AM, dverma wrote:
> - Format fixes, cleaned up the print statement
> - Style fixes, e.g. changed "if not x in y" to "if x not in y"
> - Improved variable names
> ---

Missing a Signed-off-by: tag; without that, we can't accept the patch.
Can you please resubmit with that fixed?  More hints at
https://wiki.qemu.org/index.php/Contribute/SubmitAPatch

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v3 0/3] Vmstate-static-checker.py fix upstream

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 1502124810-6930-1-git-send-email-dverma@redhat.com
Subject: [Qemu-devel] [PATCH v3 0/3] Vmstate-static-checker.py fix upstream
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c567dc828a Add new functions for whitelisting and their calls
75858f876b Update the existing whitelist
655c6dc64f Fix format and styles; make code more pythonic

=== OUTPUT BEGIN ===
Checking PATCH 1/3: Fix format and styles; make code more pythonic...
WARNING: line over 80 characters
#214: FILE: scripts/vmstate-static-checker.py:358:
+                  'minimum version error: ' + str(src_ver["version_id"]) + ' < ' +

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 245 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/3: Update the existing whitelist...
WARNING: line over 80 characters
#34: FILE: scripts/vmstate-static-checker.py:98:
+        'e1000e': ['PCIDevice', 'PCIEDevice', 'intr_state', 'redhat_7_3_intr_state'],

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 40 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/3: Add new functions for whitelisting and their calls...
ERROR: line over 90 characters
#46: FILE: scripts/vmstate-static-checker.py:64:
+# A lot of errors are generated due to differences in sizes some of which are false positives. This list

WARNING: line over 80 characters
#58: FILE: scripts/vmstate-static-checker.py:76:
+    if (old_size in new_sizes_list[field] and new_size in new_sizes_list[field]):

WARNING: line over 80 characters
#139: FILE: scripts/vmstate-static-checker.py:390:
+            # commit 20daa90a20d, extra field 'config' was added in newer releases

WARNING: line over 80 characters
#140: FILE: scripts/vmstate-static-checker.py:391:
+            # there will be a mismatch in the number of fields of irq_state and config

WARNING: line over 80 characters
#194: FILE: scripts/vmstate-static-checker.py:471:
+            check_fields(src_desc[field], dest_desc[field], src_desc["name"], sec)

WARNING: line over 80 characters
#199: FILE: scripts/vmstate-static-checker.py:474:
+            check_subsections(src_desc[field], dest_desc[field], src_desc["name"], sec)

WARNING: line over 80 characters
#211: FILE: scripts/vmstate-static-checker.py:479:
+        if not check_updated_sizes(sec, src_ver["version_id"], dest_ver["version_id"]):

ERROR: line over 90 characters
#215: FILE: scripts/vmstate-static-checker.py:483:
+            print ('version error: ' + str(src_ver["version_id"]) + ' > ' + str(dest_ver["version_id"]))

ERROR: line over 90 characters
#229: FILE: scripts/vmstate-static-checker.py:490:
+        if not check_updated_sizes(sec, src_ver["version_id"], dest_ver["minimum_version_id"]):

WARNING: line over 80 characters
#233: FILE: scripts/vmstate-static-checker.py:494:
+                      'minimum version error: ' + str(src_ver["version_id"]) + ' < ' +

WARNING: line over 80 characters
#257: FILE: scripts/vmstate-static-checker.py:509:
+            print ('size mismatch: ' + str(src["size"]) + ' , ' + str(dest["size"]))

ERROR: Missing Signed-off-by: line(s)

total: 4 errors, 8 warnings, 269 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org