[coreboot] [PATCH]Move option table to CBFS

Mathias Krause mathias.krause at secunet.com
Tue Jan 18 15:22:39 CET 2011


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




More information about the coreboot mailing list