Add basic CBFS command to list and read files #3
Conversation
| @@ -34,4 +34,11 @@ memcpy_width( | |||
| mem_op_t op | |||
| ); | |||
|
|
|||
| extern void | |||
| copy_physical( | |||
There was a problem hiding this comment.
Why do you remap /dev/mem each time rather than mapping the flash rom region once and then memcpy from the mapped region?
This isn't a performance critical piece, so this doesn't really matter...
There was a problem hiding this comment.
Sorry just laziness - will fix this up on the next commit along with @kakaroto's comments
util.c
Outdated
|
|
||
| memcpy(dest, buf, len); | ||
|
|
||
| munmap((uint8_t *)buf, len); |
There was a problem hiding this comment.
This isn't guaranteed to unmap all of the mapped memory. If phys_addr isn't page aligned or len is not a multiple of page length, map_physical() will fix them up and map a larger amount of memory.
There was a problem hiding this comment.
Good catch - I'll fix this up.
cbfs.c
Outdated
| "\n" | ||
| "-h | -? | --help This help\n" | ||
| "-v | --verbose Increase verbosity\n" | ||
| "-r | --read file Read the CBFS filed and dump to stdout\n" |
cbfs.c
Outdated
| "-v | --verbose Increase verbosity\n" | ||
| "-r | --read file Read the CBFS filed and dump to stdout\n" | ||
| "-l | --list List the files of specified type in the CBFS\n" | ||
| "-t | --type 50 Filter to CBFS file type (hex, default is raw=50)\n" |
There was a problem hiding this comment.
All types are accepted if -t is not specified (!do_type || (do_type && file.type == type)) so there is technically no default value here.
| { | ||
| fprintf(stderr, "%s", usage); | ||
| return EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
I don't know what coding style you prefer, but I would say keep the variable declarations grouped, and have this if (argc <= 1) after the filename declaration... or maybe it's not even needed actually, but instead you need to after the while getopt :
if (!do_list && !do_read) { usage }
| { | ||
| fprintf(stderr, "%s: Excess arguments?\n", prog_name); | ||
| return EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
As commented above, this might be a good place to ensure that at least a -l or a -r was passed as argument, otherwise you read the cbfs file with no action to perform.
cbfs.c
Outdated
|
|
||
| uint64_t header_off; | ||
| if (header_delta < 0) { | ||
| header_off = end - ((uint64_t)(-header_delta)); |
There was a problem hiding this comment.
wouldn't this just be : header_off = header_delta ?
There was a problem hiding this comment.
Sorry I was aggressively following this - will fix: https://github.com/coreboot/coreboot/blob/ae423852c2a533d41e7912dad6b46594c4bf8f04/util/board_status/go/src/cbfs/cbfs.go#L172
cbfs.c
Outdated
| if (file_data == NULL) { | ||
| fprintf(stderr, "Failed to allocate memory for file data: length=%d\n", | ||
| file.len); | ||
| return EXIT_FAILURE; |
There was a problem hiding this comment.
Not particularly critical since you exit right here, but I'd add free(name)
| if (rc <= 0) { | ||
| fprintf(stderr, "Failed to write file to stdout: %s\n", | ||
| strerror(errno)); | ||
| return EXIT_FAILURE; |
There was a problem hiding this comment.
Not particularly critical since you exit right here, but I'd add free(name); free(flie_data)
cbfs.c
Outdated
| copy_physical(file_off, file.len, file_data); | ||
|
|
||
| for (size_t offset = 0 ; offset < file.len ; ) { | ||
| const ssize_t rc = write( |
There was a problem hiding this comment.
I'm not entirely sure here why you would loop and write the file to stdout unless you expect the write to get interrupted by a signal. I would just do a write and make sure that rc == file.len.
Also, I agree with what @osresearch said, it's better to mmap the entire (header_off <-> end) region once and work on it rather than do here a malloc, mmap, memcpy, munmap, write, free, instead of just write. You'd need to check that 'file_off + file.len < end' (that's a sanity check I would add even if you keep the copy_physical stuff).
There was a problem hiding this comment.
Pipes often cause this problem with writes (and reads), so looping is unfortunately necessary.
|
Any other comments here? @osresearch would you like me to change the |
|
Nope! Can this be merged so linuxboot/heads#357 and linuxboot/heads#376 can move forward? :) |
|
Testing with
|
Tested on qemu and x230.