[coreboot] [PATCH]Move option table to CBFS

[Patrick Georgi]

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:
> 1. 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 :-)

> 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.

> > +		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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20110118/5707cdab/attachment.sig>