Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6465018:
Add load_firmware_test utility program (Closed)
|
Created:
9 years, 10 months ago by Che-Liang Chiou Modified:
9 years ago CC:
chromium-os-reviews_chromium.org, Randall Spangler, Luigi Semenzato, gauravsh, Bill Richardson Visibility:
Public. |
DescriptionAdd load_firmware_test utility program
BUG=chromium-os:1302
TEST=emerge vboot_reference &&
(load_firmware_test firmware_image.bin | grep LOAD_FIRMWARE_SUCCESS)
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=305e9e5
Patch Set 1 #
Total comments: 46
Patch Set 2 : Code review & remove tabs #
Total comments: 42
Patch Set 3 : Code review #
Total comments: 2
Patch Set 4 : Code review #
Messages
Total messages: 12 (0 generated)
I've added my nits. Randall? http://codereview.chromium.org/6465018/diff/1/utility/Makefile File utility/Makefile (right): http://codereview.chromium.org/6465018/diff/1/utility/Makefile#newcode67 utility/Makefile:67: $(CC) $(CFLAGS) $(INCLUDES) $< fmap.c -o $@ $(LIBS) -lcrypto Replace "$< fmap.c" with just "$^" http://codereview.chromium.org/6465018/diff/1/utility/Makefile#newcode120 utility/Makefile:120: $(CC) $(CFLAGS) $< fmap.c -o $@ Replace "$< fmap.c" with just "$^"
http://codereview.chromium.org/6465018/diff/1/utility/fmap.c File utility/fmap.c (right): http://codereview.chromium.org/6465018/diff/1/utility/fmap.c#newcode1 utility/fmap.c:1: /* Move this to host/lib Or firmware/lib if you're using these routines in U-Boot. http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h File utility/include/fmap.h (right): http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode1 utility/include/fmap.h:1: /* move this to host/include. On second thought, put it in firmware/include if you're using the same struct in U-Boot. http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode32 utility/include/fmap.h:32: } __attribute__((packed)) AreaHeader; FmapAreaHeader (using Fmap prefix will avoid naming conflicts) http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode35 utility/include/fmap.h:35: /* Return NULL if fail */ 1) More description needed. 2) Use const char*; you're not modifying the data. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c File utility/load_firmware_test.c (right): http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:26: uint8_t *fw; Google style nit: * is next to the type, not the variable so char* foo, not char *foo (vboot_reference is a little inconsistent here because I started it the other way, but we should fix these when we're changing code and use Google style for new code) http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:36: int area_index(FmapHeader *fh, AreaHeader *ah, const char name[]); move to fmap.h; add more description http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:154: int area_index(FmapHeader *fh, AreaHeader *ah, const char name[]) { Move these to fmap.c Use Fmap prefix on all function names and CamelCase, so int FmapAreaIndex(...) http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:154: int area_index(FmapHeader *fh, AreaHeader *ah, const char name[]) { const char* name http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:155: size_t len = strlen(name); Is this just an optimization to save doing the implicit strlen() multiple times? Is this a performance-critical piece of code where the gain is worth the added code? http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:215: fd = open(image_path, O_RDONLY); Why not just use ReadFile() from host lib? Code would be smaller and easier to read (and you could get rid of error_and_exit())
drive-by... (some comments may be duplicates of what Randall already mentioned. sorry about that.) http://codereview.chromium.org/6465018/diff/1/utility/Makefile File utility/Makefile (right): http://codereview.chromium.org/6465018/diff/1/utility/Makefile#newcode1 utility/Makefile:1: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved. (optional nit): update copyright year http://codereview.chromium.org/6465018/diff/1/utility/dump_fmap.c File utility/dump_fmap.c (right): http://codereview.chromium.org/6465018/diff/1/utility/dump_fmap.c#newcode153 utility/dump_fmap.c:153: s = find_fmap((char *) base_of_rom, sb.st_size); change variable name 's' to something more informative http://codereview.chromium.org/6465018/diff/1/utility/fmap.c File utility/fmap.c (right): http://codereview.chromium.org/6465018/diff/1/utility/fmap.c#newcode14 utility/fmap.c:14: char *find_fmap(char *ptr, size_t size) since this is just a library, move to */lib http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h File utility/include/fmap.h (right): http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode18 utility/include/fmap.h:18: char fmap_signature[FMAP_SIGLEN]; /* avoiding endian issues */ this comment is unclear? what does it mean? http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode36 utility/include/fmap.h:36: char *find_fmap(char *ptr, size_t size); comment about what it returns, what is ptr, what is size? http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c File utility/load_firmware_test.c (right): http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:56: int drive_load_firmware(void *base_of_rom, void *fmap) { comment on arguments and what the function does? http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:92: gbb = base_of_rom + ah[i].area_offset; 2 comments - 1) this method is pretty big. consider splitting it. 2) there are no comments on what it is doing. please document the high level blocks. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:149: EXIT: in general, we try to avoid goto. refactor to not use it. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:164: int area_index_or_error(FmapHeader *fh, AreaHeader *ah, const char name[]) { this looks like it could be moved to fmap.c http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:172: const char *status_string(int status) { comment? http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:221: base_of_rom = mmap(0, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0); i don't think it is really any better to use mmap() here. Just read the whole image into memory.
Thanks for comments. Please see below. http://codereview.chromium.org/6465018/diff/1/utility/Makefile File utility/Makefile (right): http://codereview.chromium.org/6465018/diff/1/utility/Makefile#newcode1 utility/Makefile:1: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved. On 2011/02/09 19:58:22, gauravsh wrote: > (optional nit): update copyright year Done. http://codereview.chromium.org/6465018/diff/1/utility/Makefile#newcode67 utility/Makefile:67: $(CC) $(CFLAGS) $(INCLUDES) $< fmap.c -o $@ $(LIBS) -lcrypto On 2011/02/09 16:01:46, Bill Richardson wrote: > Replace "$< fmap.c" with just "$^" > Done. Move fmap.c to host/lib. http://codereview.chromium.org/6465018/diff/1/utility/Makefile#newcode120 utility/Makefile:120: $(CC) $(CFLAGS) $< fmap.c -o $@ On 2011/02/09 16:01:46, Bill Richardson wrote: > Replace "$< fmap.c" with just "$^" Done. Move fmap.c to host/lib. http://codereview.chromium.org/6465018/diff/1/utility/dump_fmap.c File utility/dump_fmap.c (right): http://codereview.chromium.org/6465018/diff/1/utility/dump_fmap.c#newcode153 utility/dump_fmap.c:153: s = find_fmap((char *) base_of_rom, sb.st_size); On 2011/02/09 19:58:22, gauravsh wrote: > change variable name 's' to something more informative Done. http://codereview.chromium.org/6465018/diff/1/utility/fmap.c File utility/fmap.c (right): http://codereview.chromium.org/6465018/diff/1/utility/fmap.c#newcode1 utility/fmap.c:1: /* On 2011/02/09 19:42:14, Randall Spangler wrote: > Move this to host/lib > > Or firmware/lib if you're using these routines in U-Boot. Done. http://codereview.chromium.org/6465018/diff/1/utility/fmap.c#newcode14 utility/fmap.c:14: char *find_fmap(char *ptr, size_t size) On 2011/02/09 19:58:22, gauravsh wrote: > since this is just a library, move to */lib Done. http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h File utility/include/fmap.h (right): http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode1 utility/include/fmap.h:1: /* On 2011/02/09 19:42:14, Randall Spangler wrote: > move this to host/include. > > On second thought, put it in firmware/include if you're using the same struct in > U-Boot. Done. http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode18 utility/include/fmap.h:18: char fmap_signature[FMAP_SIGLEN]; /* avoiding endian issues */ On 2011/02/09 19:58:22, gauravsh wrote: > this comment is unclear? what does it mean? I am not sure. I didn't write this comment; I refactored it from dump_fmap.c. Anyway, here is my guess: The reference implementation of fmap uses uint64_t for fmap_signaure, which only works on big endian system. The guy who wrote struct _FmapHeaher copied the reference implementation, but noticed the endian issue. So he changed it to char[], and commented here. http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode32 utility/include/fmap.h:32: } __attribute__((packed)) AreaHeader; On 2011/02/09 19:42:14, Randall Spangler wrote: > FmapAreaHeader (using Fmap prefix will avoid naming conflicts) Done. http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode35 utility/include/fmap.h:35: /* Return NULL if fail */ On 2011/02/09 19:42:14, Randall Spangler wrote: > 1) More description needed. > > 2) Use const char*; you're not modifying the data. Done. http://codereview.chromium.org/6465018/diff/1/utility/include/fmap.h#newcode36 utility/include/fmap.h:36: char *find_fmap(char *ptr, size_t size); On 2011/02/09 19:58:22, gauravsh wrote: > comment about what it returns, what is ptr, what is size? Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c File utility/load_firmware_test.c (right): http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:26: uint8_t *fw; On 2011/02/09 19:42:14, Randall Spangler wrote: > Google style nit: * is next to the type, not the variable > > so char* foo, not char *foo > > (vboot_reference is a little inconsistent here because I started it the other > way, but we should fix these when we're changing code and use Google style for > new code) Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:36: int area_index(FmapHeader *fh, AreaHeader *ah, const char name[]); On 2011/02/09 19:42:14, Randall Spangler wrote: > move to fmap.h; add more description Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:56: int drive_load_firmware(void *base_of_rom, void *fmap) { On 2011/02/09 19:58:22, gauravsh wrote: > comment on arguments and what the function does? Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:92: gbb = base_of_rom + ah[i].area_offset; On 2011/02/09 19:58:22, gauravsh wrote: > 2 comments - > 1) this method is pretty big. consider splitting it. > 2) there are no comments on what it is doing. please document the high level > blocks. Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:149: EXIT: On 2011/02/09 19:58:22, gauravsh wrote: > in general, we try to avoid goto. refactor to not use it. Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:154: int area_index(FmapHeader *fh, AreaHeader *ah, const char name[]) { On 2011/02/09 19:42:14, Randall Spangler wrote: > Move these to fmap.c > > Use Fmap prefix on all function names and CamelCase, so > > int FmapAreaIndex(...) Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:154: int area_index(FmapHeader *fh, AreaHeader *ah, const char name[]) { On 2011/02/09 19:42:14, Randall Spangler wrote: > Move these to fmap.c > > Use Fmap prefix on all function names and CamelCase, so > > int FmapAreaIndex(...) Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:155: size_t len = strlen(name); On 2011/02/09 19:42:14, Randall Spangler wrote: > Is this just an optimization to save doing the implicit strlen() multiple times? > Is this a performance-critical piece of code where the gain is worth the added > code? I guess not. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:164: int area_index_or_error(FmapHeader *fh, AreaHeader *ah, const char name[]) { On 2011/02/09 19:58:22, gauravsh wrote: > this looks like it could be moved to fmap.c progname is a static global variable of load_firmware_test.c. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:172: const char *status_string(int status) { On 2011/02/09 19:58:22, gauravsh wrote: > comment? Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:215: fd = open(image_path, O_RDONLY); On 2011/02/09 19:42:14, Randall Spangler wrote: > Why not just use ReadFile() from host lib? Code would be smaller and easier to > read (and you could get rid of error_and_exit()) Done. http://codereview.chromium.org/6465018/diff/1/utility/load_firmware_test.c#ne... utility/load_firmware_test.c:221: base_of_rom = mmap(0, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0); On 2011/02/09 19:58:22, gauravsh wrote: > i don't think it is really any better to use mmap() here. Just read the whole > image into memory. Done.
Thanks for incorporating the feedback. This time I mostly have nits - around comments and style. http://codereview.chromium.org/6465018/diff/5006/host/Makefile File host/Makefile (right): http://codereview.chromium.org/6465018/diff/5006/host/Makefile#newcode1 host/Makefile:1: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved. 2011 http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h File host/include/fmap.h (right): http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode38 host/include/fmap.h:38: const char *FindFmap(const char *ptr, size_t size); char* http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode40 host/include/fmap.h:40: /* Look up fmap area by name. Retrun index of fmap area or -1 if not found. */ Retrun->Return return index into what? http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode41 host/include/fmap.h:41: int FmapAreaIndex(const FmapHeader* fh, const FmapAreaHeader* ah, still not clear from the comment what |fh| and |ah| here are. http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode44 host/include/fmap.h:44: #endif /*__FMAP_H__ */ 2 spaces here before start of comment http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c File host/lib/fmap.c (right): http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c#newcode12 host/lib/fmap.c:12: /* Return NULL if fail */ remove this comment - since you already document this in the header file http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c#newcode13 host/lib/fmap.c:13: const char *FindFmap(const char *ptr, size_t size) const char* http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c File utility/dump_fmap.c (right): http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode2 utility/dump_fmap.c:2: * Copyright (c) 2010 The Chromium OS Authors. All rights reserved. 2011 http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode22 utility/dump_fmap.c:22: static char *progname; (optional nit): for consistency with the rest of vboot_reference, please change all pointer references to use "type* varname" instead of "type *varname". if you are trying to be consistent to the rest of this file, it may be worth just cleaning the whole thing up. http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode27 utility/dump_fmap.c:27: static int dump_fmap(const void *ptr) { const void* http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode83 utility/dump_fmap.c:83: const char *fmap; const char* http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c File utility/load_firmware_test.c (right): http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:47: /* Return NULL if cannot get firmware root key */ i still don't see any comments for the arguments or what this function does. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:64: /* Return non-zero if cannot get verification block */ comment about the arguments and what this function does? http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:117: * Return zero on success, non-zero on error */ ok, I see you added the comment here - but still the rest of the functions should be properly documents especially the arguments they take. it is still not clear to me what the arguments indicate - i would mention that base_of_rom is the pointer to the start of the firmware blob, fmap is a pointer to the start of the flashmap, etc. That is not clear from the present comment. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:137: if (lfp.firmware_root_key_blob == NULL) { !lfp.firmware_root_key_blob is the usual way than explicitly comparing the pointer to NULL http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:145: /* Loop for initialize firmware key and data A / B */ s/for/to/ http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:170: /* remember to free kernel_sign_key_blob before exit */ i would get rid of the comment. it is not clear who this is supposed to help - and gives no real information to a reader of your code. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:178: /* Call to LoadFirmware */ this is not a useful comment. please remove.
http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h File host/include/fmap.h (right): http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode38 host/include/fmap.h:38: const char *FindFmap(const char *ptr, size_t size); FmapFind() would give it a consistent Fmap*() prefix with the other funcs. http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c File host/lib/fmap.c (right): http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c#newcode17 host/lib/fmap.c:17: if (0 == strncmp(ptr, "__FMAP__", 8)) Seems like "__FMAP__" should be a const string or #define in the header file, so that it can be shared between this function and whatever creates FMAPs. http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c File utility/dump_fmap.c (right): http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode30 utility/dump_fmap.c:30: const FmapHeader *fmh = (const FmapHeader *) ptr; const FmapHeader* fmh
Thanks. Please take a look. http://codereview.chromium.org/6465018/diff/5006/host/Makefile File host/Makefile (right): http://codereview.chromium.org/6465018/diff/5006/host/Makefile#newcode1 host/Makefile:1: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved. On 2011/02/10 20:02:05, gauravsh wrote: > 2011 Done. http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h File host/include/fmap.h (right): http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode38 host/include/fmap.h:38: const char *FindFmap(const char *ptr, size_t size); On 2011/02/10 20:02:05, gauravsh wrote: > char* Done. http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode38 host/include/fmap.h:38: const char *FindFmap(const char *ptr, size_t size); On 2011/02/11 17:26:04, Randall Spangler wrote: > FmapFind() would give it a consistent Fmap*() prefix with the other funcs. Done. http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode40 host/include/fmap.h:40: /* Look up fmap area by name. Retrun index of fmap area or -1 if not found. */ On 2011/02/10 20:02:05, gauravsh wrote: > Retrun->Return > > return index into what? Done. http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode41 host/include/fmap.h:41: int FmapAreaIndex(const FmapHeader* fh, const FmapAreaHeader* ah, On 2011/02/10 20:02:05, gauravsh wrote: > still not clear from the comment what |fh| and |ah| here are. Done. P.S. They are FmapHeader and FmapAreaHeader. http://codereview.chromium.org/6465018/diff/5006/host/include/fmap.h#newcode44 host/include/fmap.h:44: #endif /*__FMAP_H__ */ On 2011/02/10 20:02:05, gauravsh wrote: > 2 spaces here before start of comment Done. http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c File host/lib/fmap.c (right): http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c#newcode12 host/lib/fmap.c:12: /* Return NULL if fail */ On 2011/02/10 20:02:05, gauravsh wrote: > remove this comment - since you already document this in the header file Done. http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c#newcode13 host/lib/fmap.c:13: const char *FindFmap(const char *ptr, size_t size) On 2011/02/10 20:02:05, gauravsh wrote: > const char* Done. http://codereview.chromium.org/6465018/diff/5006/host/lib/fmap.c#newcode17 host/lib/fmap.c:17: if (0 == strncmp(ptr, "__FMAP__", 8)) On 2011/02/11 17:26:04, Randall Spangler wrote: > Seems like "__FMAP__" should be a const string or #define in the header file, so > that it can be shared between this function and whatever creates FMAPs. Done. http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c File utility/dump_fmap.c (right): http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode2 utility/dump_fmap.c:2: * Copyright (c) 2010 The Chromium OS Authors. All rights reserved. On 2011/02/10 20:02:05, gauravsh wrote: > 2011 Done. http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode22 utility/dump_fmap.c:22: static char *progname; On 2011/02/10 20:02:05, gauravsh wrote: > (optional nit): for consistency with the rest of vboot_reference, please change > all pointer references to use "type* varname" instead of "type *varname". > > if you are trying to be consistent to the rest of this file, it may be worth > just cleaning the whole thing up. Done. http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode27 utility/dump_fmap.c:27: static int dump_fmap(const void *ptr) { On 2011/02/10 20:02:05, gauravsh wrote: > const void* Done. http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode30 utility/dump_fmap.c:30: const FmapHeader *fmh = (const FmapHeader *) ptr; On 2011/02/11 17:26:04, Randall Spangler wrote: > const FmapHeader* fmh Done. http://codereview.chromium.org/6465018/diff/5006/utility/dump_fmap.c#newcode83 utility/dump_fmap.c:83: const char *fmap; On 2011/02/10 20:02:05, gauravsh wrote: > const char* Done. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c File utility/load_firmware_test.c (right): http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:47: /* Return NULL if cannot get firmware root key */ On 2011/02/10 20:02:05, gauravsh wrote: > i still don't see any comments for the arguments or what this function does. Done. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:64: /* Return non-zero if cannot get verification block */ On 2011/02/10 20:02:05, gauravsh wrote: > comment about the arguments and what this function does? Done. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:117: * Return zero on success, non-zero on error */ On 2011/02/10 20:02:05, gauravsh wrote: > ok, I see you added the comment here - but still the rest of the functions > should be properly documents especially the arguments they take. > > it is still not clear to me what the arguments indicate - i would mention that > base_of_rom is the pointer to the start of the firmware blob, fmap is a pointer > to the start of the flashmap, etc. That is not clear from the present comment. Done. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:137: if (lfp.firmware_root_key_blob == NULL) { On 2011/02/10 20:02:05, gauravsh wrote: > !lfp.firmware_root_key_blob is the usual way than explicitly comparing the > pointer to NULL Done. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:145: /* Loop for initialize firmware key and data A / B */ On 2011/02/10 20:02:05, gauravsh wrote: > s/for/to/ Done. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:170: /* remember to free kernel_sign_key_blob before exit */ On 2011/02/10 20:02:05, gauravsh wrote: > i would get rid of the comment. it is not clear who this is supposed to help - > and gives no real information to a reader of your code. Done. http://codereview.chromium.org/6465018/diff/5006/utility/load_firmware_test.c... utility/load_firmware_test.c:178: /* Call to LoadFirmware */ On 2011/02/10 20:02:05, gauravsh wrote: > this is not a useful comment. please remove. Done.
LGTM, Thanks!
LGTM with one more change. http://codereview.chromium.org/6465018/diff/15001/host/lib/fmap.c File host/lib/fmap.c (right): http://codereview.chromium.org/6465018/diff/15001/host/lib/fmap.c#newcode16 host/lib/fmap.c:16: if (0 == strncmp(ptr, FMAP_SIGNATURE, 8)) Change 8 --> FMAP_SIGNATURE_SIZE
Thanks for comments. http://codereview.chromium.org/6465018/diff/15001/host/lib/fmap.c File host/lib/fmap.c (right): http://codereview.chromium.org/6465018/diff/15001/host/lib/fmap.c#newcode16 host/lib/fmap.c:16: if (0 == strncmp(ptr, FMAP_SIGNATURE, 8)) On 2011/02/14 17:42:21, Randall Spangler wrote: > Change 8 --> FMAP_SIGNATURE_SIZE > Done.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
