Re: [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

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