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

[Carl-Daniel Hailfinger]

On 15.06.2010 21:25, Patrick Georgi wrote:
> attached patch moves functions out of the huge libpayload.h into headers
> according to libc/posix traditions, to simplify porting applications to
> payloads.
>   

Very much appreciated. This makes working with libpayload a lot easier.


> It also adds a couple of functions:
> strcasecmp, strncasecmp, strcat, strtol, strspn, strcspn, strtok_r,
> strtok, perror, exit, getpagesize
>   

Could you make perror() a wrapper for strerror()? flashrom will move to
strerror() soon to behave more like a library.


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


> +int strtol(const char *ptr, char **endptr, int base)
>   

strtol is long int, not int.


> +char *strcat(char *d, const char *s)
>   

What about more descriptive argument names?
char *strcat(char *dest, const char *src);


> +size_t strspn(const char *s, const char *a)
>   

size_t strspn(const char *s, const char *accept);


> +size_t strcspn(const char *s, const char *a)
>   

size_t strcspn(const char *s, const char *reject);


> +char* strtok_r(char *str, const char *delim, char **ptr)

char *strtok_r(char *str, const char *delim, char **saveptr);


> Index: include/pci.h
> ===================================================================
> --- include/pci.h	(Revision 5631)
> +++ include/pci.h	(Arbeitskopie)
> @@ -36,8 +36,11 @@
>  
>  #define REG_VENDOR_ID   0x00
>  #define REG_COMMAND     0x04
> +#define REG_CLASS_DEV   0x0A
>  #define REG_HEADER_TYPE 0x0E
>  #define REG_PRIMARY_BUS 0x18
> +#define REG_SUBSYS_VENDOR_ID	0x2C
> +#define REG_SUBSYS_ID	0x2E
>  
>  #define REG_COMMAND_BM  (1 << 2)
>  
>   

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.


> Index: libc/lib.c
> ===================================================================
> --- libc/lib.c	(Revision 5631)
> +++ libc/lib.c	(Arbeitskopie)
> @@ -113,3 +113,13 @@
>  	halt();
>  }
>  
> +void exit(int status)
> +{
> +	printf("exited with status %d\n", status);
> +	halt();
> +}
> +
> +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.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/