After all the other appetizers, here is the main course - LAR support for libpayload. This allows the payload to walk the lar and get information about the headers. I also added some functions modeled on fopen() and friends, but I haven't tried those since I don't have any reason to open the LAR. The functions are loosely modeled on their POSIX cousins. Examples for how to make all of this work coming next in the form of a coreinfo module.
Jordan
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
On 26/04/08 12:22 +0200, Peter Stuge wrote:
..like this. :)
- return (struct larent *) &_larent;
+}
Is it/will it become a problem that this function is not re-entrant?
Possibly. The question is if we are interested in handling "nested" LARs or not. Personally, I have thought about it a great deal, and I don't see an advantage to it - why nest a LAR within a LAR when you can nest a regular AR, or a CPIO or such. But in this code so far, I've left open the possiblity of nested LARs. In that case, this might be a issue.
This is obviously modeled on readdir(), which says:
The data returned by readdir() may be overwritten by subsequent calls to readdir() for the same directory stream.
So we should probably do it by stream if we are to have nested LARs. The problem is that the MAX_PATHLEN for LAR names is 1024, so struct larent * is pretty big, and so having a copy of it in every LAR structure would be unfortunate.
Jordan
On 26/04/08 12:22 +0200, Peter Stuge wrote:
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 ?
Fixed.
+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. :)
I think you'll find that input validation happens in all of the API functions - I think thats sufficient. The API functions should be able to verify that everything is good before sending them to the static helper functions.
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.
Done. But I'm returning 0 - no LAR is not a failure in this case.
+static void *get_header_by_name(struct LAR *lar, const char *name) +{
- struct lar_header *header;
Why not return struct lar_header * ?
Fixed.
+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 +=
Typo. Fixed;
- case SEEK_END:
file->offset = file->size - offset;break;Should this not be file->size + offset?
Yeah, but SEEK_END doesn't work anyway, so I changed it to return -1. You can't seek past the end of a LAR.
Great work, but I'm not ready to ack yet.
I'll wait for other comments before reposting a patch.
Jordan
On Sat, Apr 26, 2008 at 09:17:26AM -0600, Jordan Crouse wrote:
How about input validation? It is done here in closelar() but not in other functions.
I think you'll find that input validation happens in all of the API functions - I think thats sufficient. The API functions should be able to verify that everything is good before sending them to the static helper functions.
Aha! Yes agreed.
- case SEEK_END:
file->offset = file->size - offset;break;Should this not be file->size + offset?
Yeah, but SEEK_END doesn't work anyway, so I changed it to return -1.
Oh, no, please bring it back.
You can't seek past the end of a LAR.
A negative offset should work just fine though, and there is bounds checking and adjustment before returning.
//Peter
-
Jordan Crouse -
Peter Stuge