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 <[email protected]> > 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/ -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
