Will Bradley has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31385 )
Change subject: util/spdtool: Add tool to extract SPD from BLOBs ......................................................................
Patch Set 3:
(8 comments)
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!
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py File util/spdtool/spdtool.py:
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py@1 PS3, 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
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py@91 PS3, 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....
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py@99 PS3, 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(): ...
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py@149 PS3, 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.
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py@155 PS3, Line 155: if offset + 0x141 > len(self.blob): This should also be >=
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py@161 PS3, Line 161: if offset + 0x7d > len(self.blob): Again here
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py@193 PS3, Line 193: offset = 0 : length = 0 Dead code can be deleted.
https://review.coreboot.org/#/c/31385/3/util/spdtool/spdtool.py@212 PS3, 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 ...