On 18.01.2011 14:17, Patrick Georgi wrote:
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.
Well, but after the build the CBFS could still be modified (intentional or not) in such a way that it would no longer contain a file named "cmos_layout.bin". So at least to aid debugging it would be nice to see a warning here instead of wondering why the CMOS layout is missing in the LB table.
- 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.
Well, it's POSIX. But to support GNU Hurd I guess it's save to use an arbitrary, but sane value for TMPFILE_LEN instead of PATH_MAX.
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)
I guess you assume it'll return either 1 (good case) or 0 (bad case) not -1 - which is the case btw. But you're right. I mixed up the size and count parameters of the fwrite() call. So you check is fine the way it is.
Mathias