Re: [coreboot] libpayload: Add LAR walking support

[Peter Stuge]

On Fri, Apr 25, 2008 at 05:22:01PM -0600, Jordan Crouse wrote:
> +struct LAR *openlar(void *addr)

..
> lar = calloc(sizeof(struct LAR), 1);
> if (lar)
> lar->start = addr;
> /* Preallocate 16 slots in the cache - this saves wear and
> * tear on the heap */
> lar->headers = malloc(16 * sizeof(void *));
> lar->alloc = 16;
> lar->count = lar->eof = 0;
> lar->cindex = 0;
> return lar;
>
> +}

This is not nice if calloc() fails. Maybe add

if (!lar)
  return lar;

instead of the if(lar) before lar->start=addr ?
> +int closelar(struct LAR *lar)
> +{
> if (lar) {
> if (lar->headers)
> 	free(lar->headers);
> free(lar);
> }
> return 0;
>
> +}

How about input validation? It is done here in closelar() but not in
other functions. It would be nice to be consistent and validate
everywhere. :)

Also suggest

if (!lar)
  return -1;

right at the start of the function rather than indenting lots of code
that also covers the common case.
> +struct larent *readlar(struct LAR *lar)
> +{
> static struct larent _larent;
> struct lar_header *header;
> int nlen;
> if (!lar)
> return NULL;

..like this. :)
> return (struct larent *) &_larent;
>
> +}

Is it/will it become a problem that this function is not re-entrant?
> +static void *get_header_by_name(struct LAR *lar, const char *name)
> +{
> struct lar_header *header;

Why not return struct lar_header * ?
> +struct LFILE * lfopen(struct LAR *lar, const char *filename)
> +{
> struct LFILE *file;
> struct lar_header *header = get_header_by_name(lar, filename);
> if (header == NULL)
> return NULL;
> /* FIXME: What other validations do we want to do on the file here? */
> file = malloc(sizeof(struct LFILE));

This is re-entrant though.
> +int lfseek(struct LFILE *file, long offset, int whence)
> +{
> switch(whence) {
> case SEEK_SET:
> file->offset = offset;
> break;
> case SEEK_CUR:
> file->offset =+ offset;

Does that work? I've only evern seen +=
> case SEEK_END:
> file->offset = file->size - offset;
> break;

Should this not be file->size + offset?

Great work, but I'm not ready to ack yet.

//Peter