Am Dienstag, den 18.01.2011, 13:52 +0100 schrieb Mathias Krause:
if (option_table) {struct lb_record *rec_dest = lb_new_record(head);/* Copy the option config table, it's already a lb_record... */memcpy(rec_dest, &option_table, option_table.size);/* Create cmos checksum entry in coreboot table */lb_cmos_checksum(head);}Maybe emit a warning when no layout file was found, dispite there should have been one.
That would be a broken build (as soon as USE_OPTION_TABLE is in, cmos_layout.bin is generated and added, or should be). I'd rather make that a build time test, at runtime it's too late for the user to do something sensible about it.
- printf("--bin = Output a binary file with the definitions.\n");
^^^^^^^ that should be "--binary", I guess.
Uh, right.
- if(binary) {
int err=0;strncpy(tempfilename, dirname(strdup(binary)), TMPFILE_LEN);strncat(tempfilename, TMPFILE_TEMPLATE, TMPFILE_LEN);This looks odd. Three problems here in only two lines of code:
- The memory allocated by strdup() gets leaked. Maybe that's okay because it's only a few bytes.
And the tool is short lived. A few bytes wasted for 0.02 seconds? pff :-)
- The strncpy() isn't guaranteed to null-terminate the buffer, so strncat() may start writing way beyond the end of the buffer.
- The length argument to strncat() should be TMPFILE_LEN - strlen(tempfilename) otherwise strncat is allowed to write strlen(tempfilename) bytes beyond the end of the buffer.
Albeit looking around in the file this pattern can also be found in two other places. So maybe this should be fixed in a follow up patch.
Not related to you patch, but noticed while reviewing it: Additionally TMPFILE_LEN looks like to be choosen a little bit too small (only 256). Why isn't it something more reasonable like PATH_MAX? Maybe this should be fixed in a follow up patch, too.
PATH_MAX isn't that good an idea either, see http://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html and the blog article they link to. I guess TMPFILE_LEN used to work because it usually ends up as a short name in /tmp/, 20 characters or less. But you're right, this needs fixing.
if(!fwrite(cmos_table, (int)(ct->size-1), 1, fp)) {It would be better to check that all bytes have been written not only some, so do something like:
if(fwrite(cmos_table, (int)(ct->size-1), 1, fp) != ct->size-1) {
fwrite returns the number of successful elements, not bytes. Given that there's only 1 element to write (of size ct->size-1), I'd expect it to return a number <= 1 (1 on success, 0 or less on error)
Patrick