[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