[coreboot] [PATCH]libpayload: libc-style headers, some more functions

Patrick Georgi patrick at georgi-clan.de
Wed Jun 16 11:36:43 CEST 2010


Am 16.06.2010 09:35, schrieb Carl-Daniel Hailfinger:
> Could you make perror() a wrapper for strerror()? flashrom will move to
> strerror() soon to behave more like a library.
We don't have sterror, so I simply let it print the error code (which
isn't set anywhere). This is stubbed until there's actually some real
user of errno.

>> Signed-off-by: Patrick Georgi <patrick.georgi at coresystems.de>
> I didn't review the code in depth, so my comments may be incomplete.
> Did you write those functions from scratch, or did you merge proven code
> from a BSD licensed codebase?
I adapted libpayload functions where possible (strtoul -> strtol, for
example). Everything else is written from scratch.

> strtol is long int, not int.
I'll change that for both strtol and strtoul, thanks.

>> +char *strcat(char *d, const char *s)
> What about more descriptive argument names?
I adopted naming style (ie. single character names) from the other
functions (strncat for strcat, specifically).

This should be done globally, in a separate patch, in my opinion.

>> Index: include/pci.h
> Here is seems you're using both spaces and tabs between the identifier
> and the value. Apart from that, I found that some places use spaces for
> indentation instead of tabs.
Fixed.

>> +int getpagesize(void)
>> +{
>> +	return 4096;
>>   
> 
> Maybe wrap that in a check for x86. OTOH, if this function is used to
> determine mmap granularity, it should return 1 instead of 4096.
I stuck with the meaning of the function (and x86 has 4k pages by default).
Instead of #ifdef-hell, I moved the function to arch/*/virtual.c - both
x86 and powerpc claim 4k pages now.


Patrick




More information about the coreboot mailing list