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. >> 2. The strncpy() isn't guaranteed to null-terminate the buffer, so >> strncat() may start writing way beyond the end of the buffer. >> 3. 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 -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot