Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1701017:
Add more test cases for GptInit() and fixed some bugs (Closed)
DescriptionAdd more test cases for GptInit() and fixed some bugs
Patch Set 1 #
Total comments: 94
Patch Set 2 : refine according to Gaurav's code review #
Total comments: 17
Patch Set 3 : refine for code review #Messages
Total messages: 9 (0 generated)
Hi Louis, Here is a first pass of comments. Some high level comments: 1) There is considerable code duplication, some of which can be easily avoided (by using an array of pointers for example). I have pointed specific cases in the review. 2) Move the test case expectations to a separate file since they clutter up the actual tests and don't convey particularly useful information (if I was trying to figure out what a test does). 3) Avoid using complex multi-line macros which use conditionals and/or loops. Avoid using macros which have side-effects on unnamed arguments. 4) Move any utility like function (e.g. your sorting implementation) not related to GPT to a separate file gpt_utils.c or something. Also, functions like the quicksort implementation must definitely have associated unit tests since they are very error-prone to get right. http://codereview.chromium.org/1701017/diff/1/2 File src/platform/vboot_reference/cgptlib/cgpt.c (right): http://codereview.chromium.org/1701017/diff/1/2#newcode15 src/platform/vboot_reference/cgptlib/cgpt.c:15: } while (0) i don't quite understand the use of while(0) here? If this only needs to be run once, then why have the do-while at all? or am I missing something? http://codereview.chromium.org/1701017/diff/1/2#newcode22 src/platform/vboot_reference/cgptlib/cgpt.c:22: * ported to package. */ instead of porting a complete library, are there any existing simple CRC32 implementations that we can use here? http://codereview.chromium.org/1701017/diff/1/2#newcode29 src/platform/vboot_reference/cgptlib/cgpt.c:29: if (len >= 3) buf ^= start[i-2] << 16; add parentheses around the right hand side operands http://codereview.chromium.org/1701017/diff/1/2#newcode38 src/platform/vboot_reference/cgptlib/cgpt.c:38: * larger sector. */ large sectors http://codereview.chromium.org/1701017/diff/1/2#newcode42 src/platform/vboot_reference/cgptlib/cgpt.c:42: /* Drive sector number must be larger than basic structure for GPT. */ this comment isn't clear. Do you mean the that the drive sector number must be outside the range covered by the GPT data structure? http://codereview.chromium.org/1701017/diff/1/2#newcode62 src/platform/vboot_reference/cgptlib/cgpt.c:62: BAD_HEADER(PRIMARY); It's not clear if someone missed the macro defintion in the beginning of the file, that this macro will affect the value of valid_headers. In general, having macros having side-effects which are not clear (since valid_headers is not an argument to it)is a bad idea. Maybe redefine as - INVALIDATE_HEADER(valid_headers, index)? http://codereview.chromium.org/1701017/diff/1/2#newcode70 src/platform/vboot_reference/cgptlib/cgpt.c:70: /* Checks revision */ this comment is not very informative and unnecessary given the name of the function. This should mention what is meant by checking the revision (i.e. that they are equal to GPT_HEADER_REVISION) http://codereview.chromium.org/1701017/diff/1/2#newcode86 src/platform/vboot_reference/cgptlib/cgpt.c:86: /* Checks size of header */ Ditto for this comment too. Comments should convey more information than just function name. http://codereview.chromium.org/1701017/diff/1/2#newcode104 src/platform/vboot_reference/cgptlib/cgpt.c:104: /* Reserved fields should be zero. */ since you also check if the padding is non-zero - should the comment (and function name) reflect that too? http://codereview.chromium.org/1701017/diff/1/2#newcode120 src/platform/vboot_reference/cgptlib/cgpt.c:120: /* Checks if my_lba is configured correctly. */ Describe what it means that my_lba is configured correctly. Also what is my_lba? http://codereview.chromium.org/1701017/diff/1/2#newcode144 src/platform/vboot_reference/cgptlib/cgpt.c:144: if (primary_header->size_of_entry < MIN_SIZE_OF_ENTRY || nit: i typically prefer parenthesizing each of the boolean conditional subexpressions so as to not having to worry about operator precedence rules http://codereview.chromium.org/1701017/diff/1/2#newcode212 src/platform/vboot_reference/cgptlib/cgpt.c:212: end_of_primary_entries = 0 + GPT_PMBR_SECTOR + GPT_HEADER_SECTOR + why is the 0 needed? http://codereview.chromium.org/1701017/diff/1/2#newcode245 src/platform/vboot_reference/cgptlib/cgpt.c:245: primary_header = (GptHeader*)gpt->primary_header; It seems to me that all these tests perform the same checks on the primary header and the secondary header can you refactor them so that you have an array of pointers to the primary and secondary header and you loop through them. This would remove all the code duplication (and reduce the lines of code to half!) Something like: GptHeader *headers[2] = {gpt->primary_header, gpt->secondary_header}; for (i = 0; i < 2; i++) { // Do tests // Update valid_header bit mask based on result } http://codereview.chromium.org/1701017/diff/1/2#newcode332 src/platform/vboot_reference/cgptlib/cgpt.c:332: /* SortPairs() is actually a in-place and unstable quick sort implementation s/a/an/ http://codereview.chromium.org/1701017/diff/1/2#newcode342 src/platform/vboot_reference/cgptlib/cgpt.c:342: int Partition(pair_t* pairs, int left, int right, int pivat) { s/pivat/pivot/ http://codereview.chromium.org/1701017/diff/1/2#newcode345 src/platform/vboot_reference/cgptlib/cgpt.c:345: SWAP(right, pivat); /* 'right' now becomes 'pivat'. */ s/pivat/pivot/ http://codereview.chromium.org/1701017/diff/1/2#newcode355 src/platform/vboot_reference/cgptlib/cgpt.c:355: void SortPairs(pair_t* pairs, int left, int right) { this should be defined static. and there should be unit test for this. It maybe useful to move this outside the gpt library (to gpt_utils.c or something) if it's not GPT specific http://codereview.chromium.org/1701017/diff/1/2#newcode401 src/platform/vboot_reference/cgptlib/cgpt.c:401: BAD_ENTRIES(PRIMARY);; two ;;s http://codereview.chromium.org/1701017/diff/1/2#newcode433 src/platform/vboot_reference/cgptlib/cgpt.c:433: /* Two headers are NOT bitwise identical. We only care the following fields: s/the/about the/ http://codereview.chromium.org/1701017/diff/1/2#newcode441 src/platform/vboot_reference/cgptlib/cgpt.c:441: * If any above field are not matched, overwrite secondary with primary since we s/any above/any of above/ http://codereview.chromium.org/1701017/diff/1/2#newcode446 src/platform/vboot_reference/cgptlib/cgpt.c:446: a->last_usable_lba == b->last_usable_lba && parenthesize the subexpressions http://codereview.chromium.org/1701017/diff/1/2#newcode451 src/platform/vboot_reference/cgptlib/cgpt.c:451: else no need for the else here http://codereview.chromium.org/1701017/diff/1/2#newcode455 src/platform/vboot_reference/cgptlib/cgpt.c:455: void CopySynonymousParts(GptHeader* target, const GptHeader* source) { comment? http://codereview.chromium.org/1701017/diff/1/2#newcode463 src/platform/vboot_reference/cgptlib/cgpt.c:463: uint8_t RepairHeader(GptData_t *gpt, const uint32_t valid_headers) { comment? http://codereview.chromium.org/1701017/diff/1/3 File src/platform/vboot_reference/cgptlib/cgpt.h (right): http://codereview.chromium.org/1701017/diff/1/3#newcode35 src/platform/vboot_reference/cgptlib/cgpt.h:35: #define MIX_SIZE_OF_HEADER 92 MIN_SIZE_... http://codereview.chromium.org/1701017/diff/1/3#newcode47 src/platform/vboot_reference/cgptlib/cgpt.h:47: #define GPT_ENTRIES_SECTOR 32 /* assume sector size if 512 bytes, then would these be better named _SECTORS? http://codereview.chromium.org/1701017/diff/1/3#newcode58 src/platform/vboot_reference/cgptlib/cgpt.h:58: MASK_BOTH =3, nit: space between = and 3 http://codereview.chromium.org/1701017/diff/1/3#newcode123 src/platform/vboot_reference/cgptlib/cgpt.h:123: /* Internal use only. If these are meant to be internal and not to be used - then it'd be better to remove their prototype definitions from the header and make them static in the .c source file. http://codereview.chromium.org/1701017/diff/1/4 File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right): http://codereview.chromium.org/1701017/diff/1/4#newcode80 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:80: GptData_t* GetAClearGptData() { why do you need to create a new typedef out of GptData. Why not use just one of them everywhere? Also, in my opinion, I think a better name for this function would GetEmptyGptDta()or even GetClearedGptData() http://codereview.chromium.org/1701017/diff/1/4#newcode97 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:97: /* Fills in most of fields and creates the layout described in the top of this Specify in the comment that the argument must be a pointer to an already allocated GptData structure. In general, all function comments should contain a description of the arguments they take and their return values (if any). http://codereview.chromium.org/1701017/diff/1/4#newcode137 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:137: Memcpy(header2, header, sizeof(*header)); sizeof(GptHeader) http://codereview.chromium.org/1701017/diff/1/4#newcode175 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:175: /* Tests the default structure returned by BuildTestGptData() is good. */ s/the/if the/ http://codereview.chromium.org/1701017/diff/1/4#newcode184 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:184: /* Tests if sector_bytes is checked. is checked by what, and for what? http://codereview.chromium.org/1701017/diff/1/4#newcode186 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:186: * Future, we may support other size. */ Nit: In the future sizes http://codereview.chromium.org/1701017/diff/1/4#newcode242 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:242: /* change every char in signature of primary and secondary. Expect fail. */ s/fail/failure/ http://codereview.chromium.org/1701017/diff/1/4#newcode253 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:253: int RevisionTest() { comment? http://codereview.chromium.org/1701017/diff/1/4#newcode275 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:275: BuildTestGptData(gpt); again, I see a lot of opportunity to avoid code duplication here. All the tests follow this basic template. - modify primary - perform test - modify secondary - perform test - modify both - perform test One way would be to move the modification of the partitions outside the test function and make the test driver do the necessary bookkeeping and set test expectations. What do you think? http://codereview.chromium.org/1701017/diff/1/4#newcode865 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:865: {1, {{1, 200, 299}, {1, 100, 199}, {1, 300, 399}, {1, 100, 399}, {0, 0, 0}}}, 80 char http://codereview.chromium.org/1701017/diff/1/4#newcode866 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:866: {0, {{1, 200, 299}, {1, 100, 199}, {1, 300, 399}, {0, 100, 399}, {0, 0, 0}}}, 80 char http://codereview.chromium.org/1701017/diff/1/4#newcode867 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:867: {1, {{1, 200, 300}, {1, 100, 200}, {1, 100, 400}, {1, 300, 400}, {0, 0, 0}}}, 80 char http://codereview.chromium.org/1701017/diff/1/4#newcode868 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:868: {1, {{0, 200, 300}, {1, 100, 200}, {1, 100, 400}, {1, 300, 400}, {0, 0, 0}}}, 80 char http://codereview.chromium.org/1701017/diff/1/4#newcode869 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:869: {0, {{1, 200, 300}, {1, 100, 199}, {0, 100, 400}, {0, 300, 400}, {0, 0, 0}}}, 80 char http://codereview.chromium.org/1701017/diff/1/4#newcode878 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:878: {1, 207, 207}, {1, 208, 208}, {0, 199, 199}, {0, 0, 0}}}, It might be useful to move all the test cases (since they are data) to a separate file. Name them as TestName_cases[], so that these not clutter up the main test definitions. http://codereview.chromium.org/1701017/diff/1/4#newcode996 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:996: } else use parentheses around the else section (since you used it for if) http://codereview.chromium.org/1701017/diff/1/5 File src/platform/vboot_reference/cgptlib/tests/cgpt_test.h (right): http://codereview.chromium.org/1701017/diff/1/5#newcode6 src/platform/vboot_reference/cgptlib/tests/cgpt_test.h:6: #define VBOOT_REFERENCE_CGPTLIB_TESTS_CGPT_TEST_H_ it's ok to just use the top level module name as the prefix/ So, VBOOT_REFERENCE_CGPT_TEST_H_ is ok
Hi Gaurav, Thanks for your great reviews. I have refined my CL according to your valuable comments. After the re-arch, the code doesn't reduce too many, but looks more pretty now. Please have a look on my new code. Thank you again. http://codereview.chromium.org/1701017/diff/1/2 File src/platform/vboot_reference/cgptlib/cgpt.c (right): http://codereview.chromium.org/1701017/diff/1/2#newcode15 src/platform/vboot_reference/cgptlib/cgpt.c:15: } while (0) Oh, that's a better way than just { ... }, see http://stackoverflow.com/questions/257418/do-while-0-what-is-it-good-for On 2010/04/28 23:47:33, gauravsh wrote: > i don't quite understand the use of while(0) here? If this only needs to be run > once, then why have the do-while at all? or am I missing something? http://codereview.chromium.org/1701017/diff/1/2#newcode22 src/platform/vboot_reference/cgptlib/cgpt.c:22: * ported to package. */ On 2010/04/28 23:47:33, gauravsh wrote: > instead of porting a complete library, are there any existing simple CRC32 > implementations that we can use here? Done. http://codereview.chromium.org/1701017/diff/1/2#newcode29 src/platform/vboot_reference/cgptlib/cgpt.c:29: if (len >= 3) buf ^= start[i-2] << 16; On 2010/04/28 23:47:33, gauravsh wrote: > add parentheses around the right hand side operands Done. http://codereview.chromium.org/1701017/diff/1/2#newcode38 src/platform/vboot_reference/cgptlib/cgpt.c:38: * larger sector. */ On 2010/04/28 23:47:33, gauravsh wrote: > large sectors Done. http://codereview.chromium.org/1701017/diff/1/2#newcode42 src/platform/vboot_reference/cgptlib/cgpt.c:42: /* Drive sector number must be larger than basic structure for GPT. */ On 2010/04/28 23:47:33, gauravsh wrote: > this comment isn't clear. Do you mean the that the drive sector number must be > outside the range covered by the GPT data structure? Done. http://codereview.chromium.org/1701017/diff/1/2#newcode62 src/platform/vboot_reference/cgptlib/cgpt.c:62: BAD_HEADER(PRIMARY); On 2010/04/28 23:47:33, gauravsh wrote: > It's not clear if someone missed the macro defintion in the beginning of the > file, that this macro will affect the value of valid_headers. > In general, having macros having side-effects which are not clear (since > valid_headers is not an argument to it)is a bad idea. > > Maybe redefine as - INVALIDATE_HEADER(valid_headers, index)? Done. http://codereview.chromium.org/1701017/diff/1/2#newcode70 src/platform/vboot_reference/cgptlib/cgpt.c:70: /* Checks revision */ On 2010/04/28 23:47:33, gauravsh wrote: > this comment is not very informative and unnecessary given the name of the > function. This should mention what is meant by checking the revision (i.e. that > they are equal to GPT_HEADER_REVISION) Done. http://codereview.chromium.org/1701017/diff/1/2#newcode86 src/platform/vboot_reference/cgptlib/cgpt.c:86: /* Checks size of header */ On 2010/04/28 23:47:33, gauravsh wrote: > Ditto for this comment too. Comments should convey more information than just > function name. Done. http://codereview.chromium.org/1701017/diff/1/2#newcode104 src/platform/vboot_reference/cgptlib/cgpt.c:104: /* Reserved fields should be zero. */ On 2010/04/28 23:47:33, gauravsh wrote: > since you also check if the padding is non-zero - should the comment (and > function name) reflect that too? Done. http://codereview.chromium.org/1701017/diff/1/2#newcode120 src/platform/vboot_reference/cgptlib/cgpt.c:120: /* Checks if my_lba is configured correctly. */ On 2010/04/28 23:47:33, gauravsh wrote: > Describe what it means that my_lba is configured correctly. Also what is my_lba? Done. http://codereview.chromium.org/1701017/diff/1/2#newcode144 src/platform/vboot_reference/cgptlib/cgpt.c:144: if (primary_header->size_of_entry < MIN_SIZE_OF_ENTRY || On 2010/04/28 23:47:33, gauravsh wrote: > nit: i typically prefer parenthesizing each of the boolean conditional > subexpressions so as to not having to worry about operator precedence rules Done. http://codereview.chromium.org/1701017/diff/1/2#newcode212 src/platform/vboot_reference/cgptlib/cgpt.c:212: end_of_primary_entries = 0 + GPT_PMBR_SECTOR + GPT_HEADER_SECTOR + 0 means the index of PMBR. hm... let's remove it. On 2010/04/28 23:47:33, gauravsh wrote: > why is the 0 needed? http://codereview.chromium.org/1701017/diff/1/2#newcode245 src/platform/vboot_reference/cgptlib/cgpt.c:245: primary_header = (GptHeader*)gpt->primary_header; On 2010/04/28 23:47:33, gauravsh wrote: > It seems to me that all these tests perform the same checks on the primary > header and the secondary header > > can you refactor them so that you have an array of pointers to the primary and > secondary header and you loop through them. This would remove all the code > duplication (and reduce the lines of code to half!) > > Something like: > > GptHeader *headers[2] = {gpt->primary_header, gpt->secondary_header}; > for (i = 0; i < 2; i++) > { > // Do tests > // Update valid_header bit mask based on result > } Done. http://codereview.chromium.org/1701017/diff/1/2#newcode332 src/platform/vboot_reference/cgptlib/cgpt.c:332: /* SortPairs() is actually a in-place and unstable quick sort implementation On 2010/04/28 23:47:33, gauravsh wrote: > s/a/an/ Done. http://codereview.chromium.org/1701017/diff/1/2#newcode342 src/platform/vboot_reference/cgptlib/cgpt.c:342: int Partition(pair_t* pairs, int left, int right, int pivat) { On 2010/04/28 23:47:33, gauravsh wrote: > s/pivat/pivot/ Done. http://codereview.chromium.org/1701017/diff/1/2#newcode345 src/platform/vboot_reference/cgptlib/cgpt.c:345: SWAP(right, pivat); /* 'right' now becomes 'pivat'. */ On 2010/04/28 23:47:33, gauravsh wrote: > s/pivat/pivot/ > Done. http://codereview.chromium.org/1701017/diff/1/2#newcode355 src/platform/vboot_reference/cgptlib/cgpt.c:355: void SortPairs(pair_t* pairs, int left, int right) { On 2010/04/28 23:47:33, gauravsh wrote: > this should be defined static. and there should be unit test for this. It maybe > useful to move this outside the gpt library (to gpt_utils.c or something) if > it's not GPT specific Done. http://codereview.chromium.org/1701017/diff/1/2#newcode401 src/platform/vboot_reference/cgptlib/cgpt.c:401: BAD_ENTRIES(PRIMARY);; On 2010/04/28 23:47:33, gauravsh wrote: > two ;;s Done. http://codereview.chromium.org/1701017/diff/1/2#newcode433 src/platform/vboot_reference/cgptlib/cgpt.c:433: /* Two headers are NOT bitwise identical. We only care the following fields: On 2010/04/28 23:47:33, gauravsh wrote: > s/the/about the/ Done. http://codereview.chromium.org/1701017/diff/1/2#newcode441 src/platform/vboot_reference/cgptlib/cgpt.c:441: * If any above field are not matched, overwrite secondary with primary since we On 2010/04/28 23:47:33, gauravsh wrote: > s/any above/any of above/ Done. http://codereview.chromium.org/1701017/diff/1/2#newcode446 src/platform/vboot_reference/cgptlib/cgpt.c:446: a->last_usable_lba == b->last_usable_lba && On 2010/04/28 23:47:33, gauravsh wrote: > parenthesize the subexpressions Done. http://codereview.chromium.org/1701017/diff/1/2#newcode451 src/platform/vboot_reference/cgptlib/cgpt.c:451: else On 2010/04/28 23:47:33, gauravsh wrote: > no need for the else here Done. http://codereview.chromium.org/1701017/diff/1/2#newcode455 src/platform/vboot_reference/cgptlib/cgpt.c:455: void CopySynonymousParts(GptHeader* target, const GptHeader* source) { On 2010/04/28 23:47:33, gauravsh wrote: > comment? Done. http://codereview.chromium.org/1701017/diff/1/2#newcode463 src/platform/vboot_reference/cgptlib/cgpt.c:463: uint8_t RepairHeader(GptData_t *gpt, const uint32_t valid_headers) { On 2010/04/28 23:47:33, gauravsh wrote: > comment? Done. http://codereview.chromium.org/1701017/diff/1/3 File src/platform/vboot_reference/cgptlib/cgpt.h (right): http://codereview.chromium.org/1701017/diff/1/3#newcode35 src/platform/vboot_reference/cgptlib/cgpt.h:35: #define MIX_SIZE_OF_HEADER 92 On 2010/04/28 23:47:33, gauravsh wrote: > MIN_SIZE_... Done. http://codereview.chromium.org/1701017/diff/1/3#newcode47 src/platform/vboot_reference/cgptlib/cgpt.h:47: #define GPT_ENTRIES_SECTOR 32 /* assume sector size if 512 bytes, then On 2010/04/28 23:47:33, gauravsh wrote: > would these be better named _SECTORS? Done. http://codereview.chromium.org/1701017/diff/1/3#newcode58 src/platform/vboot_reference/cgptlib/cgpt.h:58: MASK_BOTH =3, On 2010/04/28 23:47:33, gauravsh wrote: > nit: space between = and 3 Done. http://codereview.chromium.org/1701017/diff/1/3#newcode123 src/platform/vboot_reference/cgptlib/cgpt.h:123: /* Internal use only. Hm... but those functions will be called by unittest. On 2010/04/28 23:47:33, gauravsh wrote: > If these are meant to be internal and not to be used - then it'd be better to > remove their prototype definitions from the header and make them static in the > .c source file. http://codereview.chromium.org/1701017/diff/1/4 File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right): http://codereview.chromium.org/1701017/diff/1/4#newcode80 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:80: GptData_t* GetAClearGptData() { Just because GptData_t is shorter than 'struct GptData'. Changed to GetEmptyGptData(). Thanks. On 2010/04/28 23:47:33, gauravsh wrote: > why do you need to create a new typedef out of GptData. Why not use just one of > them everywhere? > > Also, in my opinion, I think a better name for this function would > GetEmptyGptDta()or even GetClearedGptData() http://codereview.chromium.org/1701017/diff/1/4#newcode97 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:97: /* Fills in most of fields and creates the layout described in the top of this On 2010/04/28 23:47:33, gauravsh wrote: > Specify in the comment that the argument must be a pointer to an already > allocated GptData structure. In general, all function comments should contain a > description of the arguments they take and their return values (if any). Done. http://codereview.chromium.org/1701017/diff/1/4#newcode137 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:137: Memcpy(header2, header, sizeof(*header)); On 2010/04/28 23:47:33, gauravsh wrote: > sizeof(GptHeader) Done. http://codereview.chromium.org/1701017/diff/1/4#newcode175 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:175: /* Tests the default structure returned by BuildTestGptData() is good. */ On 2010/04/28 23:47:33, gauravsh wrote: > s/the/if the/ Done. http://codereview.chromium.org/1701017/diff/1/4#newcode184 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:184: /* Tests if sector_bytes is checked. On 2010/04/28 23:47:33, gauravsh wrote: > is checked by what, and for what? Done. http://codereview.chromium.org/1701017/diff/1/4#newcode186 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:186: * Future, we may support other size. */ On 2010/04/28 23:47:33, gauravsh wrote: > Nit: In the future > > sizes Done. http://codereview.chromium.org/1701017/diff/1/4#newcode242 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:242: /* change every char in signature of primary and secondary. Expect fail. */ On 2010/04/28 23:47:33, gauravsh wrote: > s/fail/failure/ Done. http://codereview.chromium.org/1701017/diff/1/4#newcode253 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:253: int RevisionTest() { On 2010/04/28 23:47:33, gauravsh wrote: > comment? Done. http://codereview.chromium.org/1701017/diff/1/4#newcode275 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:275: BuildTestGptData(gpt); On 2010/04/28 23:47:33, gauravsh wrote: > again, I see a lot of opportunity to avoid code duplication here. All the tests > follow this basic template. > - modify primary > - perform test > - modify secondary > - perform test > - modify both > - perform test > > One way would be to move the modification of the partitions outside the test > function and make the test driver do the necessary bookkeeping and set test > expectations. What do you think? Done. http://codereview.chromium.org/1701017/diff/1/4#newcode865 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:865: {1, {{1, 200, 299}, {1, 100, 199}, {1, 300, 399}, {1, 100, 399}, {0, 0, 0}}}, On 2010/04/28 23:47:33, gauravsh wrote: > 80 char > Done. http://codereview.chromium.org/1701017/diff/1/4#newcode866 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:866: {0, {{1, 200, 299}, {1, 100, 199}, {1, 300, 399}, {0, 100, 399}, {0, 0, 0}}}, On 2010/04/28 23:47:33, gauravsh wrote: > 80 char Done. http://codereview.chromium.org/1701017/diff/1/4#newcode867 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:867: {1, {{1, 200, 300}, {1, 100, 200}, {1, 100, 400}, {1, 300, 400}, {0, 0, 0}}}, On 2010/04/28 23:47:33, gauravsh wrote: > 80 char Done. http://codereview.chromium.org/1701017/diff/1/4#newcode868 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:868: {1, {{0, 200, 300}, {1, 100, 200}, {1, 100, 400}, {1, 300, 400}, {0, 0, 0}}}, On 2010/04/28 23:47:33, gauravsh wrote: > 80 char Done. http://codereview.chromium.org/1701017/diff/1/4#newcode869 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:869: {0, {{1, 200, 300}, {1, 100, 199}, {0, 100, 400}, {0, 300, 400}, {0, 0, 0}}}, On 2010/04/28 23:47:33, gauravsh wrote: > 80 char Done. http://codereview.chromium.org/1701017/diff/1/4#newcode878 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:878: {1, 207, 207}, {1, 208, 208}, {0, 199, 199}, {0, 0, 0}}}, hm... I think moving them out is over-killing. Keeping them in this function is easier to read (locality) although they are ugly. On 2010/04/28 23:47:33, gauravsh wrote: > It might be useful to move all the test cases (since they are data) to a > separate file. Name them as TestName_cases[], so that these not clutter up the > main test definitions. http://codereview.chromium.org/1701017/diff/1/4#newcode996 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:996: } else On 2010/04/28 23:47:33, gauravsh wrote: > use parentheses around the else section (since you used it for if) Done. http://codereview.chromium.org/1701017/diff/1/5 File src/platform/vboot_reference/cgptlib/tests/cgpt_test.h (right): http://codereview.chromium.org/1701017/diff/1/5#newcode6 src/platform/vboot_reference/cgptlib/tests/cgpt_test.h:6: #define VBOOT_REFERENCE_CGPTLIB_TESTS_CGPT_TEST_H_ On 2010/04/28 23:47:33, gauravsh wrote: > it's ok to just use the top level module name as the prefix/ So, > VBOOT_REFERENCE_CGPT_TEST_H_ is ok Done.
Overall this is looking very good. I mostly only have minor nits now. And thanks for adding the tests for your quicksort implementation. http://codereview.chromium.org/1701017/diff/1/3 File src/platform/vboot_reference/cgptlib/cgpt.h (right): http://codereview.chromium.org/1701017/diff/1/3#newcode123 src/platform/vboot_reference/cgptlib/cgpt.h:123: /* Internal use only. Can they be moved to a separate cgpt_internal.h header then? The reason for my suggestion is that cgpt.h should only expose the actual usable API. You can have cgpt.c include both cgpt.h and cgpt_internal.h. Your choice though. On 2010/04/30 01:34:55, Louis wrote: > Hm... but those functions will be called by unittest. > > On 2010/04/28 23:47:33, gauravsh wrote: > > If these are meant to be internal and not to be used - then it'd be better to > > remove their prototype definitions from the header and make them static in the > > .c source file. > > http://codereview.chromium.org/1701017/diff/1/4 File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right): http://codereview.chromium.org/1701017/diff/1/4#newcode80 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:80: GptData_t* GetAClearGptData() { So why not just typedef struct GptData to GptData? My comment was mainly about consistency - use one of struct GptData or GptData_t everywhere (see, for example, the RefereshCRC function above is using struct GptData). On 2010/04/30 01:34:55, Louis wrote: > Just because GptData_t is shorter than 'struct GptData'. > > Changed to GetEmptyGptData(). Thanks. > On 2010/04/28 23:47:33, gauravsh wrote: > > why do you need to create a new typedef out of GptData. Why not use just one > of > > them everywhere? > > > > Also, in my opinion, I think a better name for this function would > > GetEmptyGptDta()or even GetClearedGptData() > > http://codereview.chromium.org/1701017/diff/7001/8001 File src/platform/vboot_reference/cgptlib/cgpt.c (right): http://codereview.chromium.org/1701017/diff/7001/8001#newcode26 src/platform/vboot_reference/cgptlib/cgpt.c:26: * large sector. */ oops, i meant larger sectors :) http://codereview.chromium.org/1701017/diff/7001/8001#newcode230 src/platform/vboot_reference/cgptlib/cgpt.c:230: (GptHeader*)gpt->primary_header, This cast (GptHeader*) is unnecessary (assuming gpt->primary_header is defined as GptHeader*) http://codereview.chromium.org/1701017/diff/7001/8001#newcode421 src/platform/vboot_reference/cgptlib/cgpt.c:421: * If both headers are valid (CRC32 is correct) but they are not synonymous, I think it might be confusing in the context to some what synonymous means - functionally equivalent? logically equivalent? Can you say it some other way - say, the primary fields are the same or something like that. http://codereview.chromium.org/1701017/diff/7001/8003 File src/platform/vboot_reference/cgptlib/crc32.c (right): http://codereview.chromium.org/1701017/diff/7001/8003#newcode2 src/platform/vboot_reference/cgptlib/crc32.c:2: /* COPYRIGHT (C) 1986 Gary S. Brown. You may use this program, or */ I don't know the exact format to follow here - but I think you should have the chromium os copyright notice and then mention, the source of the implementation, where you got it from. And then say - "the original copyright notice follows". Look in vboot_reference/cryptolibs/sha2.c for an example. http://codereview.chromium.org/1701017/diff/7001/8007 File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right): http://codereview.chromium.org/1701017/diff/7001/8007#newcode93 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:93: * file. Before calling this function, primary/secondary header/entries must have 80 chars http://codereview.chromium.org/1701017/diff/7001/8007#newcode94 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:94: * been pointed to the buffer, says gpt returned from GetEmptyGptData(). say, a gpt returned from.. http://codereview.chromium.org/1701017/diff/7001/8009 File src/platform/vboot_reference/cgptlib/tests/quick_sort_test.c (right): http://codereview.chromium.org/1701017/diff/7001/8009#newcode53 src/platform/vboot_reference/cgptlib/tests/quick_sort_test.c:53: { compare: ascent_compare, verify: ascent_verify, }, btw, have you made sure that this structure field initialization syntax is ANSI C compliant? It doesn't so much matter for the quick sort unittests, but I remember seeing it used in cgpt.c somewhere. If this is not ANSI C compliant, then the firmware compiler may complain at this. http://codereview.chromium.org/1701017/diff/7001/8010 File src/platform/vboot_reference/cgptlib/tests/quick_sort_test.h (right): http://codereview.chromium.org/1701017/diff/7001/8010#newcode8 src/platform/vboot_reference/cgptlib/tests/quick_sort_test.h:8: int TestQuickSortFixed(); maybe a header is file not really required for the quick sort tests :)
Thank you, Gaurav. Your portability concern is right. I've removed the designated initializers. Also modified others according to your suggestions. Please have a look. Thanks for reviewing thousands lines of code again and again. http://codereview.chromium.org/1701017/diff/1/3 File src/platform/vboot_reference/cgptlib/cgpt.h (right): http://codereview.chromium.org/1701017/diff/1/3#newcode123 src/platform/vboot_reference/cgptlib/cgpt.h:123: /* Internal use only. Sure. Would love to do that. Done. On 2010/04/30 05:57:14, gauravsh wrote: > Can they be moved to a separate cgpt_internal.h header then? The reason for my > suggestion is that cgpt.h should only expose the actual usable API. You can have > cgpt.c include both cgpt.h and cgpt_internal.h. Your choice though. > > On 2010/04/30 01:34:55, Louis wrote: > > Hm... but those functions will be called by unittest. > > > > On 2010/04/28 23:47:33, gauravsh wrote: > > > If these are meant to be internal and not to be used - then it'd be better > to > > > remove their prototype definitions from the header and make them static in > the > > > .c source file. > > > > > > http://codereview.chromium.org/1701017/diff/1/4 File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right): http://codereview.chromium.org/1701017/diff/1/4#newcode80 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:80: GptData_t* GetAClearGptData() { My fault. Have unified to GptData. On 2010/04/30 05:57:14, gauravsh wrote: > So why not just typedef struct GptData to GptData? My comment was mainly about > consistency - use one of struct GptData or GptData_t everywhere (see, for > example, the RefereshCRC function above is using struct GptData). > > On 2010/04/30 01:34:55, Louis wrote: > > Just because GptData_t is shorter than 'struct GptData'. > > > > Changed to GetEmptyGptData(). Thanks. > > On 2010/04/28 23:47:33, gauravsh wrote: > > > why do you need to create a new typedef out of GptData. Why not use just one > > of > > > them everywhere? > > > > > > Also, in my opinion, I think a better name for this function would > > > GetEmptyGptDta()or even GetClearedGptData() > > > > > > http://codereview.chromium.org/1701017/diff/7001/8001 File src/platform/vboot_reference/cgptlib/cgpt.c (right): http://codereview.chromium.org/1701017/diff/7001/8001#newcode26 src/platform/vboot_reference/cgptlib/cgpt.c:26: * large sector. */ no problem. I happened to refine the style and comment. :-) On 2010/04/30 05:57:14, gauravsh wrote: > oops, i meant larger sectors :) http://codereview.chromium.org/1701017/diff/7001/8001#newcode230 src/platform/vboot_reference/cgptlib/cgpt.c:230: (GptHeader*)gpt->primary_header, gpt->primary_header is actually defined as uint8_t* so that caller doesn't have to know the implementation of GptHeader. For caller, it just loads blob from sector and calls GptInit() to handle that. On 2010/04/30 05:57:14, gauravsh wrote: > This cast (GptHeader*) is unnecessary (assuming gpt->primary_header is defined > as GptHeader*) http://codereview.chromium.org/1701017/diff/7001/8001#newcode421 src/platform/vboot_reference/cgptlib/cgpt.c:421: * If both headers are valid (CRC32 is correct) but they are not synonymous, On 2010/04/30 05:57:14, gauravsh wrote: > I think it might be confusing in the context to some what synonymous means - > functionally equivalent? logically equivalent? Can you say it some other way - > say, the primary fields are the same or something like that. Done. http://codereview.chromium.org/1701017/diff/7001/8003 File src/platform/vboot_reference/cgptlib/crc32.c (right): http://codereview.chromium.org/1701017/diff/7001/8003#newcode2 src/platform/vboot_reference/cgptlib/crc32.c:2: /* COPYRIGHT (C) 1986 Gary S. Brown. You may use this program, or */ On 2010/04/30 05:57:14, gauravsh wrote: > I don't know the exact format to follow here - but I think you should have the > chromium os copyright notice and then mention, the source of the implementation, > where you got it from. And then say - "the original copyright notice follows". > Look in vboot_reference/cryptolibs/sha2.c for an example. Done. http://codereview.chromium.org/1701017/diff/7001/8007 File src/platform/vboot_reference/cgptlib/tests/cgpt_test.c (right): http://codereview.chromium.org/1701017/diff/7001/8007#newcode93 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:93: * file. Before calling this function, primary/secondary header/entries must have On 2010/04/30 05:57:14, gauravsh wrote: > 80 chars Done. http://codereview.chromium.org/1701017/diff/7001/8007#newcode94 src/platform/vboot_reference/cgptlib/tests/cgpt_test.c:94: * been pointed to the buffer, says gpt returned from GetEmptyGptData(). On 2010/04/30 05:57:14, gauravsh wrote: > say, a gpt returned from.. Done. http://codereview.chromium.org/1701017/diff/7001/8009 File src/platform/vboot_reference/cgptlib/tests/quick_sort_test.c (right): http://codereview.chromium.org/1701017/diff/7001/8009#newcode53 src/platform/vboot_reference/cgptlib/tests/quick_sort_test.c:53: { compare: ascent_compare, verify: ascent_verify, }, ANSI C99 did support designated initializer, but let's remove it if a developer uses C89 complier. On 2010/04/30 05:57:14, gauravsh wrote: > btw, have you made sure that this structure field initialization syntax is ANSI > C compliant? It doesn't so much matter for the quick sort unittests, but I > remember seeing it used in cgpt.c somewhere. If this is not ANSI C compliant, > then the firmware compiler may complain at this. http://codereview.chromium.org/1701017/diff/7001/8010 File src/platform/vboot_reference/cgptlib/tests/quick_sort_test.h (right): http://codereview.chromium.org/1701017/diff/7001/8010#newcode8 src/platform/vboot_reference/cgptlib/tests/quick_sort_test.h:8: int TestQuickSortFixed(); I thought it could be a good convention if we always have a header file exporting variables/functions to sync up correct prototype. On 2010/04/30 05:57:14, gauravsh wrote: > maybe a header is file not really required for the quick sort tests :)
LGTM with comment. http://codereview.chromium.org/1701017/diff/7001/8001 File src/platform/vboot_reference/cgptlib/cgpt.c (right): http://codereview.chromium.org/1701017/diff/7001/8001#newcode230 src/platform/vboot_reference/cgptlib/cgpt.c:230: (GptHeader*)gpt->primary_header, On 2010/04/30 18:45:05, Louis wrote: > gpt->primary_header is actually defined as uint8_t* so that caller doesn't have > to know the implementation of GptHeader. For caller, it just loads blob from > sector and calls GptInit() to handle that. > On 2010/04/30 05:57:14, gauravsh wrote: > > This cast (GptHeader*) is unnecessary (assuming gpt->primary_header is defined > > as GptHeader*) > > Oh all right. In that case, I just want to be sure that we don't run into padding issues later on - say we generate the GptHeader on a different platform with different alignment requirements, and this type cast doesn't do what we intend. Is GptData_t something that will be used by the firmware?
I don't think so. GPT headers/entries are actually stored on hard drive. vboot is the only one, as far as I know, would read it from hard drive. On 2010/04/30 22:50:05, gauravsh wrote: > LGTM with comment. > > http://codereview.chromium.org/1701017/diff/7001/8001 > File src/platform/vboot_reference/cgptlib/cgpt.c (right): > > http://codereview.chromium.org/1701017/diff/7001/8001#newcode230 > src/platform/vboot_reference/cgptlib/cgpt.c:230: > (GptHeader*)gpt->primary_header, > On 2010/04/30 18:45:05, Louis wrote: > > gpt->primary_header is actually defined as uint8_t* so that caller doesn't > have > > to know the implementation of GptHeader. For caller, it just loads blob from > > sector and calls GptInit() to handle that. > > On 2010/04/30 05:57:14, gauravsh wrote: > > > This cast (GptHeader*) is unnecessary (assuming gpt->primary_header is > defined > > > as GptHeader*) > > > > > Oh all right. In that case, I just want to be sure that we don't run into > padding issues later on - say we generate the GptHeader on a different platform > with different alignment requirements, and this type cast doesn't do what we > intend. > > Is GptData_t something that will be used by the firmware? |
