Add helper functions and files for gpt tests.

Add helper functions and files for gpt tests.

[gauravsh, 2010-04-22 21:22:22]

http://codereview.chromium.org/1729006/diff/1/2
File src/platform/vboot_reference/cgptlib/Makefile (right):

http://codereview.chromium.org/1729006/diff/1/2#newcode1
src/platform/vboot_reference/cgptlib/Makefile:1: # Copyright (c) 2009 The
Chromium OS Authors. All rights reserved.
2010

http://codereview.chromium.org/1729006/diff/1/2#newcode5
src/platform/vboot_reference/cgptlib/Makefile:5: C = cgpt.c
What's the variable C for?

http://codereview.chromium.org/1729006/diff/1/3
File src/platform/vboot_reference/cgptlib/cgpt.c (right):

http://codereview.chromium.org/1729006/diff/1/3#newcode8
src/platform/vboot_reference/cgptlib/cgpt.c:8: #include <string.h>
sys_gpt.h should go after string.h

http://codereview.chromium.org/1729006/diff/1/3#newcode18
src/platform/vboot_reference/cgptlib/cgpt.c:18: if (memcmp(gpt->primary_header,
"EFI PART", 8))
make EFI_PART a string constant, and use sizeof() instead of hardcoding 8.

http://codereview.chromium.org/1729006/diff/1/3#newcode31
src/platform/vboot_reference/cgptlib/cgpt.c:31: gpt->current_kernel ^= 1;
again, where does this 1 come from, and why is the XOR being done?

http://codereview.chromium.org/1729006/diff/1/5
File src/platform/vboot_reference/cgptlib/sys_gpt.h (right):

http://codereview.chromium.org/1729006/diff/1/5#newcode1
src/platform/vboot_reference/cgptlib/sys_gpt.h:1: /*-
Is this file modified at all from the original version? In either case, you
might want to add a comment to the beginning stating where you got this from -
what it does, and in addition, have the full copyright notice above.

http://codereview.chromium.org/1729006/diff/1/5#newcode45
src/platform/vboot_reference/cgptlib/sys_gpt.h:45: uint32_t	hdr_crc_self;
strictly optional, but it would be better to convert whitespacing in this file
to follow google style.

http://codereview.chromium.org/1729006/diff/1/6
File src/platform/vboot_reference/cgptlib/sys_uuid.h (right):

http://codereview.chromium.org/1729006/diff/1/6#newcode1
src/platform/vboot_reference/cgptlib/sys_uuid.h:1: /*-
same comment as sys_gpt.h

http://codereview.chromium.org/1729006/diff/1/8
File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right):

http://codereview.chromium.org/1729006/diff/1/8#newcode7
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:7: #include "cgpt_test.h"
cgpt_test.h followed by string.h followed by rest
file header
c header
other includes

http://codereview.chromium.org/1729006/diff/1/8#newcode16
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:16: #define
DEFAULT_DRIVE_SECTORS 467
just curious, how do you get the 467 number?

http://codereview.chromium.org/1729006/diff/1/8#newcode53
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:53: struct GPTData*
EmptyGdpData() {
EmptyGPTData?

http://codereview.chromium.org/1729006/diff/1/8#newcode53
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:53: struct GPTData*
EmptyGdpData() {
Function Comment? Also should this be something that should be moved out of
tests to the GPT implementation?

http://codereview.chromium.org/1729006/diff/1/8#newcode61
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:61:
memset(&primary_header, 0, sizeof(primary_header));
since this stuff is going to pushed into the firmware - you might want to stub
out all standard library functions (like memset, memcpy) to something like
Memset, Memcpy. Not so important for tests, but important for actual
implementation to make it easy to move this to the firmware. look under
common/utility_stub.c to see how the vboot reference does it.

http://codereview.chromium.org/1729006/diff/1/8#newcode86
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:86: *     467
Now I see where the 467 comes from, move this to the beginning of the file -
since all the tests using this layout.

http://codereview.chromium.org/1729006/diff/1/8#newcode88
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:88: struct GPTData*
Again, should this function be moved to the GPT implementation since this will
most likely be used by the GPT API too.

http://codereview.chromium.org/1729006/diff/1/8#newcode89
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:89:
BuildGoodGdpData(uint32_t sector_bytes, uint64_t drive_sectors) {
did you mean GPTData( instead of GdpData(?

http://codereview.chromium.org/1729006/diff/1/8#newcode136
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:136: static void
dump(void *vp, int len, char* memo) {
comment?

http://codereview.chromium.org/1729006/diff/1/8#newcode159
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:159: int SignatureTest()
{
is this a signature check or a checksum (CRC) check?

http://codereview.chromium.org/1729006/diff/1/8#newcode188
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:188: /* change every char
in signature of primary. Secondary is still valid. */
comment fix? isn't this for the case when both primary and secondary are
invalid?

[Louis, 2010-04-23 01:13:21]

Thanks for review the CL. I've fixed according to your suggestion.

By the way, to integrate into vboot make system, we still need to have discuss.
We have to build library directories first, then target binary and test
binaries.

http://codereview.chromium.org/1729006/diff/1/2
File src/platform/vboot_reference/cgptlib/Makefile (right):

http://codereview.chromium.org/1729006/diff/1/2#newcode1
src/platform/vboot_reference/cgptlib/Makefile:1: # Copyright (c) 2009 The
Chromium OS Authors. All rights reserved.
On 2010/04/22 21:22:22, gauravsh wrote:
> 2010

Done.

http://codereview.chromium.org/1729006/diff/1/2#newcode5
src/platform/vboot_reference/cgptlib/Makefile:5: C = cgpt.c
Not used right now. I have removed.
On 2010/04/22 21:22:22, gauravsh wrote:
> What's the variable C for?

http://codereview.chromium.org/1729006/diff/1/3
File src/platform/vboot_reference/cgptlib/cgpt.c (right):

http://codereview.chromium.org/1729006/diff/1/3#newcode8
src/platform/vboot_reference/cgptlib/cgpt.c:8: #include <string.h>
Done.
On 2010/04/22 21:22:22, gauravsh wrote:
> sys_gpt.h should go after string.h

http://codereview.chromium.org/1729006/diff/1/3#newcode18
src/platform/vboot_reference/cgptlib/cgpt.c:18: if (memcmp(gpt->primary_header,
"EFI PART", 8))
On 2010/04/22 21:22:22, gauravsh wrote:
> make EFI_PART a string constant, and use sizeof() instead of hardcoding 8.

Done.

http://codereview.chromium.org/1729006/diff/1/3#newcode31
src/platform/vboot_reference/cgptlib/cgpt.c:31: gpt->current_kernel ^= 1;
Oooops. That's not really code, and would be fixed soon. That just returns
anything like it is moving.
On 2010/04/22 21:22:22, gauravsh wrote:
> again, where does this 1 come from, and why is the XOR being done?

http://codereview.chromium.org/1729006/diff/1/5
File src/platform/vboot_reference/cgptlib/sys_gpt.h (right):

http://codereview.chromium.org/1729006/diff/1/5#newcode1
src/platform/vboot_reference/cgptlib/sys_gpt.h:1: /*-
Since the naming convention is not compatible to coding style and not readable,
I will re-write it.
On 2010/04/22 21:22:22, gauravsh wrote:
> Is this file modified at all from the original version? In either case, you
> might want to add a comment to the beginning stating where you got this from -
> what it does, and in addition, have the full copyright notice above.

http://codereview.chromium.org/1729006/diff/1/5#newcode45
src/platform/vboot_reference/cgptlib/sys_gpt.h:45: uint32_t	hdr_crc_self;
On 2010/04/22 21:22:22, gauravsh wrote:
> strictly optional, but it would be better to convert whitespacing in this file
> to follow google style.

Done.

http://codereview.chromium.org/1729006/diff/1/6
File src/platform/vboot_reference/cgptlib/sys_uuid.h (right):

http://codereview.chromium.org/1729006/diff/1/6#newcode1
src/platform/vboot_reference/cgptlib/sys_uuid.h:1: /*-
Please ignore this file because gpt.h has replaced this.
On 2010/04/22 21:22:22, gauravsh wrote:
> same comment as sys_gpt.h

http://codereview.chromium.org/1729006/diff/1/8
File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right):

http://codereview.chromium.org/1729006/diff/1/8#newcode7
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:7: #include "cgpt_test.h"
On 2010/04/22 21:22:22, gauravsh wrote:
> cgpt_test.h followed by string.h followed by rest
> file header
> c header
> other includes

Done.

http://codereview.chromium.org/1729006/diff/1/8#newcode16
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:16: #define
DEFAULT_DRIVE_SECTORS 467
On 2010/04/22 21:22:22, gauravsh wrote:
> just curious, how do you get the 467 number?

Done.

http://codereview.chromium.org/1729006/diff/1/8#newcode53
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:53: struct GPTData*
EmptyGdpData() {
Changed to a descriptive name.
On 2010/04/22 21:22:22, gauravsh wrote:
> EmptyGPTData?

http://codereview.chromium.org/1729006/diff/1/8#newcode53
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:53: struct GPTData*
EmptyGdpData() {
Done. This is used for test only.
On 2010/04/22 21:22:22, gauravsh wrote:
> Function Comment? Also should this be something that should be moved out of
> tests to the GPT implementation?

http://codereview.chromium.org/1729006/diff/1/8#newcode61
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:61:
memset(&primary_header, 0, sizeof(primary_header));
On 2010/04/22 21:22:22, gauravsh wrote:
> since this stuff is going to pushed into the firmware - you might want to stub
> out all standard library functions (like memset, memcpy) to something like
> Memset, Memcpy. Not so important for tests, but important for actual
> implementation to make it easy to move this to the firmware. look under
> common/utility_stub.c to see how the vboot reference does it.

Done.

http://codereview.chromium.org/1729006/diff/1/8#newcode86
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:86: *     467
On 2010/04/22 21:22:22, gauravsh wrote:
> Now I see where the 467 comes from, move this to the beginning of the file -
> since all the tests using this layout.

Done.

http://codereview.chromium.org/1729006/diff/1/8#newcode88
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:88: struct GPTData*
No. This function is used by test only.
On 2010/04/22 21:22:22, gauravsh wrote:
> Again, should this function be moved to the GPT implementation since this will
> most likely be used by the GPT API too.

http://codereview.chromium.org/1729006/diff/1/8#newcode89
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:89:
BuildGoodGdpData(uint32_t sector_bytes, uint64_t drive_sectors) {
On 2010/04/22 21:22:22, gauravsh wrote:
> did you mean GPTData( instead of GdpData(?

Done.

http://codereview.chromium.org/1729006/diff/1/8#newcode136
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:136: static void
dump(void *vp, int len, char* memo) {
On 2010/04/22 21:22:22, gauravsh wrote:
> comment?

Done.

http://codereview.chromium.org/1729006/diff/1/8#newcode159
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:159: int SignatureTest()
{
In EFI spec, signature is "EFI PART".
On 2010/04/22 21:22:22, gauravsh wrote:
> is this a signature check or a checksum (CRC) check?

http://codereview.chromium.org/1729006/diff/1/8#newcode188
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:188: /* change every char
in signature of primary. Secondary is still valid. */
On 2010/04/22 21:22:22, gauravsh wrote:
> comment fix? isn't this for the case when both primary and secondary are
> invalid?

Done.

[gauravsh, 2010-04-23 01:38:59]

http://codereview.chromium.org/1729006/diff/9001/10001
File src/platform/vboot_reference/cgptlib/Makefile (right):

http://codereview.chromium.org/1729006/diff/9001/10001#newcode5
src/platform/vboot_reference/cgptlib/Makefile:5: CFLAGS ?= -Wall -Werror
does it make sense to also add a -ansi flag. Just to make sure we catch errors
that may make the firmware compiler complain.

http://codereview.chromium.org/1729006/diff/9001/10004
File src/platform/vboot_reference/cgptlib/gpt.h (right):

http://codereview.chromium.org/1729006/diff/9001/10004#newcode44
src/platform/vboot_reference/cgptlib/gpt.h:44: uint32_t revision;
Add comments for structure in the file describing their fields or link to a
document which explains the format?

http://codereview.chromium.org/1729006/diff/9001/10006
File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right):

http://codereview.chromium.org/1729006/diff/9001/10006#newcode96
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:96: * file. */
So, does this function work if drive_sectors is different than
DEFAULT_DRIVE_SECTORS(467)? It seems to me the value 467 is hard coded into the
calculations below?

Also, would a better name for this be BuildTestGPTData()?

http://codereview.chromium.org/1729006/diff/9001/10006#newcode100
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:100: GptHeader *header,
*header2;
nit: consistency with the capitalization of gpt - choose one of Gpt or GPT.

http://codereview.chromium.org/1729006/diff/9001/10008
File src/platform/vboot_reference/common/include/utility.h (right):

http://codereview.chromium.org/1729006/diff/9001/10008#newcode43
src/platform/vboot_reference/common/include/utility.h:43: /* Compare [n] bytes
in [src1] and [src2] */
What about the return value?

[Louis, 2010-04-23 03:31:37]

Thanks for review. I've fixed. Please kindly take a review. Thanks again.

http://codereview.chromium.org/1729006/diff/9001/10001
File src/platform/vboot_reference/cgptlib/Makefile (right):

http://codereview.chromium.org/1729006/diff/9001/10001#newcode5
src/platform/vboot_reference/cgptlib/Makefile:5: CFLAGS ?= -Wall -Werror
On 2010/04/23 01:38:59, gauravsh wrote:
> does it make sense to also add a -ansi flag. Just to make sure we catch errors
> that may make the firmware compiler complain.

Done.

http://codereview.chromium.org/1729006/diff/9001/10004
File src/platform/vboot_reference/cgptlib/gpt.h (right):

http://codereview.chromium.org/1729006/diff/9001/10004#newcode44
src/platform/vboot_reference/cgptlib/gpt.h:44: uint32_t revision;
On 2010/04/23 01:38:59, gauravsh wrote:
> Add comments for structure in the file describing their fields or link to a
> document which explains the format?

Done.

http://codereview.chromium.org/1729006/diff/9001/10006
File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right):

http://codereview.chromium.org/1729006/diff/9001/10006#newcode96
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:96: * file. */
On 2010/04/23 01:38:59, gauravsh wrote:
> So, does this function work if drive_sectors is different than
> DEFAULT_DRIVE_SECTORS(467)? It seems to me the value 467 is hard coded into
the
> calculations below?
> 
> Also, would a better name for this be BuildTestGPTData()?

Done.

http://codereview.chromium.org/1729006/diff/9001/10006#newcode100
src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:100: GptHeader *header,
*header2;
On 2010/04/23 01:38:59, gauravsh wrote:
> nit: consistency with the capitalization of gpt - choose one of Gpt or GPT.

Done.

http://codereview.chromium.org/1729006/diff/9001/10008
File src/platform/vboot_reference/common/include/utility.h (right):

http://codereview.chromium.org/1729006/diff/9001/10008#newcode43
src/platform/vboot_reference/common/include/utility.h:43: /* Compare [n] bytes
in [src1] and [src2] */
On 2010/04/23 01:38:59, gauravsh wrote:
> What about the return value?
> 

Done. Copied from man 3 memcmp.

[gauravsh, 2010-04-23 03:43:51]

lgtm