Hello there! I'm stepping in here with comments having never seen the rest of this codebase. So, I don't really know anything about the social aspects of code review in the context of coreboot. I was asked by a friend to take a look, so please take all of this with a grain of salt. Cheers!

8 comments:

File util/spdtool/spdtool.py:
- Patch Set #3, Line 1: #!/usr/bin/env python
  Starting comments.
  In general "/usr/bin/env python" means Python 2, so for someone to run this using Python3, they would have to explicitly invoke it with "python3 util/spdtool/spdtool.py ..."
  I'm not super familiar with this codebase, but I'll quickly dump out the warnings and errors that flake8 found in this source code. Note that flake8 is a command line tool to check for PEP8 compliance. If you are not familiar with PEP8, it is documented here: https://www.python.org/dev/peps/pep-0008/
  spdtool.py:5:1: E302 expected 2 blank lines, found 1
  spdtool.py:10:5: E301 expected 1 blank line, found 0
  spdtool.py:13:5: E301 expected 1 blank line, found 0
  spdtool.py:16:5: E301 expected 1 blank line, found 0
  spdtool.py:19:5: E301 expected 1 blank line, found 0
  spdtool.py:22:5: E301 expected 1 blank line, found 0
  spdtool.py:25:5: E301 expected 1 blank line, found 0
  spdtool.py:55:80: E501 line too long (94 > 79 characters)
  spdtool.py:70:1: E302 expected 2 blank lines, found 1
  spdtool.py:101:80: E501 line too long (91 > 79 characters)
  spdtool.py:102:80: E501 line too long (91 > 79 characters)
  spdtool.py:123:80: E501 line too long (84 > 79 characters)
  spdtool.py:126:8: E111 indentation is not a multiple of four
  spdtool.py:127:8: E111 indentation is not a multiple of four
  spdtool.py:128:12: E111 indentation is not a multiple of four
  spdtool.py:129:8: E111 indentation is not a multiple of four
  spdtool.py:129:80: E501 line too long (81 > 79 characters)
  spdtool.py:132:8: E111 indentation is not a multiple of four
  spdtool.py:133:8: E111 indentation is not a multiple of four
  spdtool.py:134:12: E111 indentation is not a multiple of four
  spdtool.py:136:8: E111 indentation is not a multiple of four
  spdtool.py:137:12: E111 indentation is not a multiple of four
  spdtool.py:138:8: E111 indentation is not a multiple of four
  spdtool.py:139:8: E111 indentation is not a multiple of four
  spdtool.py:140:8: E111 indentation is not a multiple of four
  spdtool.py:141:8: E111 indentation is not a multiple of four
  spdtool.py:142:8: E111 indentation is not a multiple of four
  spdtool.py:144:1: E305 expected 2 blank lines after class or function definition, found 1
  spdtool.py:177:17: E128 continuation line under-indented for visual indent
  spdtool.py:179:80: E501 line too long (118 > 79 characters)
  spdtool.py:179:83: E225 missing whitespace around operator
  spdtool.py:179:93: E225 missing whitespace around operator
  spdtool.py:179:103: E225 missing whitespace around operator
  spdtool.py:187:55: E231 missing whitespace after ','
  spdtool.py:187:80: E501 line too long (87 > 79 characters)
  spdtool.py:190:17: W292 no newline at end of file
- Patch Set #3, Line 91:
```
if self.blob_as_ord(offset) != i:
                    continue
                if not self.verify_match(i, offset):
                    continue
```
  nit: The multiple "continue"s followed by the return are a bit harder to read than is necessary. Why not turn these into two anded clauses that return when they are satisfied?
```
for ...:
    for ...:
        if ... and ...:
            return offset, self....
```
- Patch Set #3, Line 99: def get_matches(self):
  This function does not use "self". Consider lifting it out of the class into a private module function (leading underscore.)
```
def _get_ddr4_header_candidates():
    ...
```
- Patch Set #3, Line 149: if offset + 0x15c > len(self.blob):
  This looks like it should be >=, given that you are indexing self.blob[offset + 0x15c] below. If len(self.blob) == 0x15c and offset == 0, then self.blob[offset + 0x15c] will result in an IndexError.
- Patch Set #3, Line 155: if offset + 0x141 > len(self.blob):
  This should also be >=
- Patch Set #3, Line 161: if offset + 0x7d > len(self.blob):
  Again here
- Patch Set #3, Line 193:
```
offset = 0
    length = 0
```
  Dead code can be deleted.
- Patch Set #3, Line 212: fn = open(filename, "w")
  idiomatic Python is to use a "with" block here to ensure that all variants of Python will properly flush/close the file prior to the next loop coming along and opening the file again.
```
with open(filename, "w") as fn:
    j = 0
    for ...
```

To view, visit change 31385. To unsubscribe, or for help writing mail filters, visit settings.