check_tfs: find entries that define value_string identical to common tfs

2 files changed, 144 insertions, 13 deletions

diff --git a/epan/dissectors/packet-umts_fp.c b/epan/dissectors/packet-umts_fp.c
index 1cc0942ea8..a3a6e0600c 100644
--- a/epan/dissectors/packet-umts_fp.c
+++ b/epan/dissectors/packet-umts_fp.c
@@ -312,12 +312,6 @@ static const value_string frame_type_vals[] = {
     { 0,   NULL }
 };
 
-static const value_string direction_vals[] = {
-    { 0,   "Downlink" },
-    { 1,   "Uplink" },
-    { 0,   NULL }
-};
-
 static const value_string crci_vals[] = {
     { 0,   "Correct" },
     { 1,   "Not correct" },
@@ -5911,7 +5905,7 @@ dissect_fp_common(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void *dat
     }
 
     /* Add link direction as a generated field */
-    ti = proto_tree_add_uint(fp_tree, hf_fp_direction, tvb, 0, 0, p_fp_info->is_uplink);
+    ti = proto_tree_add_boolean(fp_tree, hf_fp_direction, tvb, 0, 0, p_fp_info->is_uplink);
     proto_item_set_generated(ti);
 
     /* Don't currently handle IuR-specific formats, but it's useful to even see
@@ -6106,7 +6100,7 @@ void proto_register_fp(void)
             },
             { &hf_fp_direction,
               { "Direction",
-                "fp.direction", FT_UINT8, BASE_HEX, VALS(direction_vals), 0x0,
+                "fp.direction", FT_BOOLEAN, 8, TFS(&tfs_uplink_downlink), 0x0,
                 "Link direction", HFILL
               }
             },
diff --git a/tools/check_tfs.py b/tools/check_tfs.py
index b1ad33e54e..19d0af0e9e 100755
--- a/tools/check_tfs.py
+++ b/tools/check_tfs.py
@@ -16,7 +16,7 @@ import signal
 
 # TODO:
 # - check how many of the definitions in epan/tfs.c are used in other dissectors
-# - see if there are other values that should be in epan/tfs.c and shared
+#      - although even if unused, might be in external dissectors?
 
 
 # Try to exit soon after Ctrl-C is pressed.
@@ -30,6 +30,37 @@ def signal_handler(sig, frame):
 signal.signal(signal.SIGINT, signal_handler)
 
 
+# Test for whether the given file was automatically generated.
+def isGeneratedFile(filename):
+    # Open file
+    f_read = open(os.path.join(filename), 'r')
+    lines_tested = 0
+    for line in f_read:
+        # The comment to say that its generated is near the top, so give up once
+        # get a few lines down.
+        if lines_tested > 10:
+            f_read.close()
+            return False
+        if (line.find('Generated automatically') != -1 or
+            line.find('Generated Automatically') != -1 or
+            line.find('Autogenerated from') != -1 or
+            line.find('is autogenerated') != -1 or
+            line.find('automatically generated by Pidl') != -1 or
+            line.find('Created by: The Qt Meta Object Compiler') != -1 or
+            line.find('This file was generated') != -1 or
+            line.find('This filter was automatically generated') != -1 or
+            line.find('This file is auto generated, do not edit!') != -1 or
+            line.find('This file is auto generated') != -1):
+
+            f_read.close()
+            return True
+        lines_tested = lines_tested + 1
+
+    # OK, looks like a hand-written file!
+    f_read.close()
+    return False
+
+
 # Keep track of custom entries that might appear in multiple dissectors,
 # so we can consider adding them to tfs.c
 custom_tfs_entries = {}
@@ -59,13 +90,50 @@ class TFS:
         return '{' + '"' + self.val1 + '", "' + self.val2 + '"}'
 
 
+class ValueString:
+    def __init__(self, file, name, vals):
+        self.file = file
+        self.name = name
+        self.raw_vals = vals
+        self.parsed_vals = {}
+        self.looks_like_tfs = True
+
+        no_lines =  self.raw_vals.count('{')
+        if no_lines != 3:
+            self.looks_like_tfs = False
+            return
+
+        # Now parse out each entry in the value_string
+        matches = re.finditer(r'\{([\"a-zA-Z\s\d\,]*)\}', self.raw_vals)
+        for m in matches:
+            entry = m[1]
+            # Check each entry looks like part of a TFS entry.
+            match = re.match(r'\s*([01])\,\s*\"([a-zA-Z\d\s]*\s*)\"', entry)
+            if match:
+                if match[1] == '1':
+                    self.parsed_vals[True] = match[2]
+                else:
+                    self.parsed_vals[False] = match[2]
+
+                # Now have both entries
+                if len(self.parsed_vals) == 2:
+                    break
+            else:
+                self.looks_like_tfs = False
+                break
+
+    def __str__(self):
+        return '{' + '"' + self.raw_vals + '"}'
+
+
+
 def removeComments(code_string):
     code_string = re.sub(re.compile(r"/\*.*?\*/",re.DOTALL ) ,"" ,code_string) # C-style comment
     code_string = re.sub(re.compile(r"//.*?\n" ) ,"" ,code_string)             # C++-style comment
     return code_string
 
 
-# Look for hf items in a dissector file.
+# Look for true_false_string items in a dissector file.
 def findItems(filename):
     items = {}
 
@@ -86,6 +154,31 @@ def findItems(filename):
 
     return items
 
+# Look for value_string entries in a dissector file.
+def findValueStrings(filename):
+    items = {}
+
+    #static const value_string radio_type_vals[] =
+    #{
+    #    { 0,      "FDD"},
+    #    { 1,      "TDD"},
+    #    { 0, NULL }
+    #};
+
+    with open(filename, 'r') as f:
+        contents = f.read()
+
+        # Remove comments so as not to trip up RE.
+        contents = removeComments(contents)
+
+        matches =   re.finditer(r'.*const value_string\s*([a-zA-Z0-9_]*)\s*\[\s*\]\s*\=\s*\{([\{\}\d\,a-zA-Z\s\"]*)\};', contents)
+        for m in matches:
+            name = m.group(1)
+            vals = m.group(2)
+            items[name] = ValueString(filename, name, vals)
+
+    return items
+
 
 
 def is_dissector_file(filename):
@@ -104,11 +197,13 @@ def findDissectorFilesInFolder(folder):
             files.append(filename)
     return files
 
+
+
 warnings_found = 0
 errors_found = 0
 
 # Check the given dissector file.
-def checkFile(filename, tfs_items, look_for_common=False):
+def checkFile(filename, tfs_items, look_for_common=False, check_value_strings=False):
     global warnings_found
     global errors_found
 
@@ -145,7 +240,7 @@ def checkFile(filename, tfs_items, look_for_common=False):
                     found = True
 
                 if found:
-                    print(filename, i, "- could have used", t, 'from tfs.c instead: ', tfs_items[t],
+                    print("Error:" if exact_case else "Warn: ", filename, i, "- could have used", t, 'from tfs.c instead: ', tfs_items[t],
                           '' if exact_case else '  (capitalisation differs)')
                     if exact_case:
                         errors_found += 1
@@ -156,6 +251,44 @@ def checkFile(filename, tfs_items, look_for_common=False):
             if look_for_common:
                 AddCustomEntry(items[i].val1, items[i].val2, filename)
 
+    if check_value_strings:
+        vs = findValueStrings(filename)
+        for v in vs:
+            if vs[v].looks_like_tfs:
+                found = False
+                exact_case = False
+
+                #print('Candidate', v, vs[v])
+                for t in tfs_items:
+                    found = False
+
+                    #
+                    # Do not do this check for plugins; plugins cannot import
+                    # data values from libwireshark (functions, yes; data
+                    # values, no).
+                    #
+                    # Test whether there's a common prefix for the file name
+                    # and "plugin/epan/"; if so, this is a plugin, and there
+                    # is no common path and os.path.commonprefix returns an
+                    # empty string, otherwise it returns the common path, so
+                    # we check whether the common path is an empty string.
+                    #
+                    if os.path.commonprefix([filename, 'plugin/epan/']) == '':
+                        exact_case = False
+                        if tfs_items[t].val1 == vs[v].parsed_vals[True] and tfs_items[t].val2 == vs[v].parsed_vals[False]:
+                            found = True
+                            exact_case = True
+                        elif tfs_items[t].val1.upper() == vs[v].parsed_vals[True].upper() and tfs_items[t].val2.upper() == vs[v].parsed_vals[False].upper():
+                            found = True
+
+                        if found:
+                            print("Warn:" if exact_case else "Note:", filename, 'value_string', v, "- could have used", t, 'from tfs.c instead: ', tfs_items[t],
+                                '' if exact_case else '  (capitalisation differs)')
+                            if exact_case:
+                                warnings_found += 1
+                            break
+
+
 
 #################################################################
 # Main logic.
@@ -169,6 +302,9 @@ parser.add_argument('--commits', action='store',
                     help='last N commits to check')
 parser.add_argument('--open', action='store_true',
                     help='check open files')
+parser.add_argument('--check-value-strings', action='store_true',
+                    help='check whether value_strings could have been tfs?')
+
 parser.add_argument('--common', action='store_true',
                     help='check for potential new entries for tfs.c')
 
@@ -234,7 +370,8 @@ tfs_entries = findItems(os.path.join('epan', 'tfs.c'))
 for f in files:
     if should_exit:
         exit(1)
-    checkFile(f, tfs_entries, look_for_common=args.common)
+    if not isGeneratedFile(f):
+        checkFile(f, tfs_entries, look_for_common=args.common, check_value_strings=args.check_value_strings)
 
 
 # Show summary.