Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(69)

Issue 2745007: Major refactoring of structures, with unit tests. (Closed)

Created:
10 years, 6 months ago by Randall Spangler
Modified:
9 years, 7 months ago
CC:
chromium-os-reviews_chromium.org
Base URL:
ssh://gitrw.chromium.org/vboot_reference.git
Visibility:
Public.

Description

Major refactoring of structures, with unit tests. This matches the doc I sent out earlier. Firmware-side code for LoadKernel() is in place now. LoadFirmware() replacement coming soon. The new functions are implemented in parallel to the existing ones (i.e., everything that used to work still does).

Patch Set 1 #

Patch Set 2 : More refactoring/cleanup #

Patch Set 3 : Ready for review #

Patch Set 4 : Pulled patches to LoadKernel() into LoadKernel2() #

Patch Set 5 : Implemented LoadFirmware2() #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+2523 lines, -36 lines) Patch
M Makefile View 1 chunk +3 lines, -1 line 0 comments Download
A host/Makefile View 1 1 chunk +45 lines, -0 lines 0 comments Download
A host/include/host_common.h View 1 chunk +51 lines, -0 lines 0 comments Download
A host/include/host_key.h View 1 chunk +59 lines, -0 lines 0 comments Download
A host/include/host_signature.h View 1 chunk +48 lines, -0 lines 0 comments Download
A host/lib/host_common.c View 1 chunk +185 lines, -0 lines 0 comments Download
A host/lib/host_key.c View 1 chunk +145 lines, -0 lines 2 comments Download
A host/lib/host_signature.c View 1 chunk +135 lines, -0 lines 0 comments Download
A host/linktest/main.c View 1 1 chunk +28 lines, -0 lines 0 comments Download
M tests/Makefile View 5 chunks +16 lines, -1 line 0 comments Download
M tests/run_image_verification_tests.sh View 1 chunk +1 line, -0 lines 0 comments Download
A tests/run_vboot_common_tests.sh View 1 1 chunk +91 lines, -0 lines 0 comments Download
A tests/vboot_common2_tests.c View 1 1 chunk +210 lines, -0 lines 0 comments Download
A tests/vboot_common3_tests.c View 1 1 chunk +295 lines, -0 lines 0 comments Download
A tests/vboot_common_tests.c View 1 chunk +101 lines, -0 lines 0 comments Download
M utility/Makefile View 1 4 chunks +20 lines, -11 lines 0 comments Download
M utility/load_kernel_test.c View 1 chunk +1 line, -1 line 0 comments Download
M vboot_firmware/Makefile View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments Download
M vboot_firmware/include/load_firmware_fw.h View 1 chunk +4 lines, -2 lines 0 comments Download
M vboot_firmware/include/load_kernel_fw.h View 2 chunks +4 lines, -5 lines 1 comment Download
M vboot_firmware/lib/cryptolib/include/rsa.h View 1 chunk +2 lines, -1 line 0 comments Download
M vboot_firmware/lib/cryptolib/rsa_utility.c View 1 chunk +1 line, -1 line 0 comments Download
A vboot_firmware/lib/include/vboot_common.h View 1 2 3 4 1 chunk +92 lines, -0 lines 0 comments Download
A vboot_firmware/lib/include/vboot_firmware.h View 1 chunk +24 lines, -0 lines 1 comment Download
A vboot_firmware/lib/include/vboot_kernel.h View 1 2 3 4 1 chunk +37 lines, -0 lines 2 comments Download
A vboot_firmware/lib/include/vboot_struct.h View 1 chunk +123 lines, -0 lines 3 comments Download
M vboot_firmware/lib/load_kernel_fw.c View 1 2 3 4 6 chunks +15 lines, -5 lines 0 comments Download
A vboot_firmware/lib/vboot_common.c View 1 chunk +312 lines, -0 lines 1 comment Download
A vboot_firmware/lib/vboot_firmware.c View 1 chunk +173 lines, -0 lines 1 comment Download
A vboot_firmware/lib/vboot_kernel.c View 1 2 3 4 1 chunk +266 lines, -0 lines 2 comments Download
M vboot_firmware/linktest/main.c View 2 3 4 4 chunks +32 lines, -7 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
Randall Spangler
10 years, 6 months ago (2010-06-10 00:35:04 UTC) #1
gauravsh
Overall, I really like this change but given the size of the CL, I am ...
10 years, 6 months ago (2010-06-10 14:44:13 UTC) #2
gauravsh
High level question about this. Are you also aiming to ensure that if the verified ...
10 years, 6 months ago (2010-06-10 16:04:26 UTC) #3
Randall Spangler
On Thu, Jun 10, 2010 at 9:04 AM, <gauravsh@chromium.org> wrote: > High level question about ...
10 years, 6 months ago (2010-06-10 16:20:55 UTC) #4
vb
10 years, 6 months ago (2010-06-10 17:37:48 UTC) #5
I just had a quick look at the common structs, a few comments/suggestions FYI.

http://codereview.chromium.org/2745007/diff/9001/10026
File vboot_firmware/lib/include/vboot_struct.h (right):

http://codereview.chromium.org/2745007/diff/9001/10026#newcode28
vboot_firmware/lib/include/vboot_struct.h:28: uint64_t sig_size;    /* Size of
signature data from start of this struct */
Shouldn't this be just 'size of signature data'?

Also, why do you need to use 8 byte fields? It might be required for offsets
(albeit I don't see files larger than 2^32 in size practical), but all other
fields could be 4 bytes and some of them - 2? Or is this to not worry about
packing?

Generally, structures shared between compilers, are better to be explicitly
packed and accompanied by some compile time checks verifying the layout.

http://codereview.chromium.org/2745007/diff/9001/10026#newcode34
vboot_firmware/lib/include/vboot_struct.h:34: #define KEY_BLOCK_MAGIC_SIZE 8
would be more robust defined as (sizeof(KEY_BLOCK_MAGIC) - 1)

Powered by Google App Engine
This is Rietveld 408576698