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

[Patrick Georgi]

Hi,

attached patch moves functions out of the huge libpayload.h into headers
according to libc/posix traditions, to simplify porting applications to
payloads.

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

baiyin cai, I added you to the recipients, as marc indicated that this
might overlap with your summer of code effort.

Comments are appreciated.

Signed-off-by: Patrick Georgi

[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

[Patrick Georgi]

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