This patch fixes lar path handling. In particular, it adds new members to the file struct for pathname and compression, so that directories can be correctly recursed and files can be renamed when being added. This fixes a buildrom breakage when adding the payload as well.
file-by-file changes:
util/lar/lar.c: make error messages more verbose pass a pointer to the file structure instead of the name start with a NULL path before parsing the name
util/lar/lib.c: change handle_directory to use a path name and respect nocompress change add_files to use lar_process_name instead of its own local version use sensible defaults for new file members when listing or extracting free pathname if allocated
util/lar/lib.h: add new members to struct file change prototypes of add_files and lar_add_file
util/lar/stream.c: change lar_add_file to use pathname and compression from struct file
Myles
Signed-off-by: Myles Watson mylesgw@gmail.com
On Mon, Feb 25, 2008 at 01:47:59PM -0700, Myles Watson wrote:
strncpy(fullname, name, MAX_PATH);
strncpy(fullpathname, pathname, MAX_PATH);
--8<-- strcpy(3) The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null terminated. -->8--
One fix is to simply memset() fullname and fullpathname at the start of the function - I don't like assuming that heap variables are zero initialized.
if (name[(strlen(name)) - 1] != '/') { strncat(fullname, "/", MAX_PATH); }
if (name[(strlen(pathname)) - 1] != '/') {strncat(fullpathname, "/", MAX_PATH);}strncat(fullname, namelist[n]->d_name, MAX_PATH);
add_files(fullname);
strncat(fullpathname, namelist[n]->d_name,MAX_PATH);add_files(fullname,fullpathname,thisalgo);
This algorithm protects against overflow, but I would like if it also raised an error when MAX_PATH isn't big enough.
+int add_files(const char *name, const char * pathname_in,
const enum compalgo algo_in)
..
ret = lar_process_name((char*)name, &filename, &pathname, &thisalgo);
Why is this cast needed? Does lar_process_name() modify name? If not please fix it's prototype so no cast is needed.
- /*printf("%s: %s (%s:%s)\n",__FUNCTION__,name,filename,pathname);*/
Is there some debug functionality in lar for these?
if (elfparse() && iself(ptr)) {
output_elf_segments(lar, pathname, ptr, size, thisalgo);
output_elf_segments(lar, file->pathname, ptr, size, file->algo);
How does this work with file->algo vs. zeroes compression?
//Peter
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Peter Stuge Sent: Monday, February 25, 2008 4:34 PM To: Coreboot Subject: Re: [coreboot] lar directory handling patch
On Mon, Feb 25, 2008 at 01:47:59PM -0700, Myles Watson wrote:
strncpy(fullname, name, MAX_PATH);
strncpy(fullpathname, pathname, MAX_PATH);--8<-- strcpy(3) The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null terminated. -->8--
One fix is to simply memset() fullname and fullpathname at the start of the function - I don't like assuming that heap variables are zero initialized.
I guess I figured that the chance of having pathnames > 1024 was pretty close to zero, so I didn't change the way it was done. I guess I could just set the last byte of the array to '\0' and only strncpy MAX_PATH-1 too.
I'll use memset.
if (name[(strlen(name)) - 1] != '/') { strncat(fullname, "/", MAX_PATH); }
if (name[(strlen(pathname)) - 1] != '/') {strncat(fullpathname, "/",
MAX_PATH);
}strncat(fullname, namelist[n]->d_name, MAX_PATH);
add_files(fullname);
strncat(fullpathname, namelist[n]->d_name,MAX_PATH);add_files(fullname,fullpathname,thisalgo);This algorithm protects against overflow, but I would like if it also raised an error when MAX_PATH isn't big enough.
Sure, I can add that. I guess I should be more careful about the math for the strncat too. It looks like I've left open the possibility of adding two paths < MAX_PATH and getting one larger than MAX_PATH. Again I guess we're hoping that no one will ever use really long directory names and deep nesting on us.
+int add_files(const char *name, const char * pathname_in,
const enum compalgo algo_in)..
ret = lar_process_name((char*)name, &filename, &pathname,&thisalgo);
Why is this cast needed? Does lar_process_name() modify name? If not please fix it's prototype so no cast is needed.
It does. It changes ':' to '\0' for files like:
nocompress:payload-filo.elf:normal/payload
- /*printf("%s: %s (%s:%s)\n",__FUNCTION__,name,filename,pathname);*/
Is there some debug functionality in lar for these?
Not that I saw. You're talking about a verbose switch or something like that?
if (elfparse() && iself(ptr)) {
output_elf_segments(lar, pathname, ptr, size, thisalgo);
output_elf_segments(lar, file->pathname, ptr, size, file-algo);
How does this work with file->algo vs. zeroes compression?
It doesn't change it. The .bss segment is treated specially, and overrides the compression mode.
//Peter
Thanks for reviewing,
Myles
On Mon, Feb 25, 2008 at 05:00:41PM -0700, Myles Watson wrote:
MAX_PATH
Sure, I can add that. I guess I should be more careful about the math for the strncat too.
Good catch.
Again I guess we're hoping that no one will ever use really long directory names and deep nesting on us.
I admit it is currently completely unlikely since coreboot doesn't care about anything deeper than two levels anyway.
+int add_files(const char *name, const char * pathname_in,
const enum compalgo algo_in)..
ret = lar_process_name((char*)name, &filename, &pathname,&thisalgo);
Why is this cast needed? Does lar_process_name() modify name? If not please fix it's prototype so no cast is needed.
It does. It changes ':' to '\0' for files like:
nocompress:payload-filo.elf:normal/payload
Ah. Then removing const from the name parameter in add_files() would be better, right?
- /*printf("%s: %s (%s:%s)\n",__FUNCTION__,name,filename,pathname);*/
Is there some debug functionality in lar for these?
Not that I saw. You're talking about a verbose switch or something like that?
Exactly. Ok.
output_elf_segments(lar, file->pathname, ptr, size,file->algo
How does this work with file->algo vs. zeroes compression?
It doesn't change it. The .bss segment is treated specially, and overrides the compression mode.
Excellent!
//Peter
Here's another try.
On Mon, Feb 25, 2008 at 4:34 PM, Peter Stuge peter@stuge.se wrote:
On Mon, Feb 25, 2008 at 01:47:59PM -0700, Myles Watson wrote:
strncpy(fullname, name, MAX_PATH);
strncpy(fullpathname, pathname, MAX_PATH);
I took out strncpy and strncat.
strncat(fullpathname, namelist[n]->d_name,MAX_PATH);add_files(fullname,fullpathname,thisalgo);This algorithm protects against overflow, but I would like if it also raised an error when MAX_PATH isn't big enough.
Fixed in the same way as the last patch I submitted. This one supersedes it.
+int add_files(const char *name, const char * pathname_in,
const enum compalgo algo_in)..
ret = lar_process_name((char*)name, &filename, &pathname, &thisalgo);Why is this cast needed? Does lar_process_name() modify name? If not please fix it's prototype so no cast is needed.
I changed it so that the options processing gets done in lar.c with the rest of the options processing, which lets it be a const char *.
/*printf("%s: %s (%s:%s)\n",__FUNCTION__,name,filename,pathname);*/Is there some debug functionality in lar for these?
Yes, I changed them to if (verbose()) printf ...
Myles
Signed-off-by: Myles Watson mylesgw@gmail.com
On Wed, Feb 27, 2008 at 1:17 PM, Myles Watson mylesgw@gmail.com wrote:
Here's another try.
On Mon, Feb 25, 2008 at 4:34 PM, Peter Stuge peter@stuge.se wrote:
On Mon, Feb 25, 2008 at 01:47:59PM -0700, Myles Watson wrote:
strncpy(fullname, name, MAX_PATH);
strncpy(fullpathname, pathname, MAX_PATH);I took out strncpy and strncat.
strncat(fullpathname, namelist[n]->d_name,
MAX_PATH);add_files(fullname,fullpathname,thisalgo);This algorithm protects against overflow, but I would like if it also raised an error when MAX_PATH isn't big enough.
Fixed in the same way as the last patch I submitted. This one supersedes it.
+int add_files(const char *name, const char * pathname_in,
const enum compalgo algo_in)..
ret = lar_process_name((char*)name, &filename, &pathname, &thisalgo);Why is this cast needed? Does lar_process_name() modify name? If not please fix it's prototype so no cast is needed.
I changed it so that the options processing gets done in lar.c with the rest of the options processing, which lets it be a const char *.
/*printf("%s: %s (%s:%s)\n",__FUNCTION__,name,filename,pathname);*/Is there some debug functionality in lar for these?
Yes, I changed them to if (verbose()) printf ...
I realized that I was adding an extra message on a failure and returning a value instead of exiting in lar.c main. Here's the updated patch.
Myles
Signed-off-by: Myles Watson mylesgw@gmail.com
On Wed, Feb 27, 2008 at 01:32:25PM -0700, Myles Watson wrote:
Here's the updated patch.
Signed-off-by: Myles Watson mylesgw@gmail.com
Like it! Just some small nits about coding style.
Acked-by: Peter Stuge peter@stuge.se
len += (name[len-1]=='/'?1:0);
Some like spaces around ? and :
- if (verbose()) printf("%s: %s:%s\n",__FUNCTION__,filename,pathname);
Separate lines and space after comma.
// Is it a directory?
C++ comment.
ret = handle_directory(filename,pathname,thisalgo);
Space after commas.
Personally I don't care about these things, terser is better for me, but I guess we should try to keep the style consistent in the repo.
//Peter
On Wed, Feb 27, 2008 at 5:11 PM, Peter Stuge peter@stuge.se wrote:
On Wed, Feb 27, 2008 at 01:32:25PM -0700, Myles Watson wrote:
Here's the updated patch.
Signed-off-by: Myles Watson mylesgw@gmail.com
Like it! Just some small nits about coding style.
Acked-by: Peter Stuge peter@stuge.se
Thanks for the review. I fixed the coding style issues. Rev 623.
Myles
-
Myles Watson -
Peter Stuge