[coreboot] lar copy patch

[Carl-Daniel Hailfinger]

Hi Myles,

I think we're moving forward and one or two iterations should give us a
mergeable patch.

On 07.03.2008 17:11, Myles Watson wrote:
>> -----Original Message-----
>> From: Carl-Daniel Hailfinger [mailto:c-d.hailfinger.devel.2006 at gmx.net]
>> Sent: Thursday, March 06, 2008 6:35 PM
>> To: Myles Watson
>> Cc: Coreboot
>> Subject: Re: [coreboot] lar copy patch
>>
>> On 07.03.2008 00:47, Myles Watson wrote:
>>     
>>> This patch adds copy functionality to lar via
>>>
>>> lar -L srclar.file
>>>
>>>       
>> Although I appreciate the work you've done, I still feel the lar copy
>> approach is not right.
>>     
>  
> The other options I've seen are:
> 	1. Invent a new file format
> 	2. Extract to ELF minus some information
>   

The "Extract to ELF" option would work if Ron had the time to do it. But
since he's busy with so many coreboot things, I can't really force him
to work on that issue as well.


>>> It allows you to use payloads, bootblocks, or other entries from one
>>> lar when creating or adding to another.
>>>
>>> changes by file:
>>> util/lar/lar.c:
>>> 	change usage
>>> 	add srcarchivename parameter for create_lar and add_lar
>>> 	add lar_copy functionality to create_lar and add_lar
>>> 	add -C keep for using the same compression as srclar
>>> util/lar/lar.h:
>>> 	change invalid to invalid_or_keep in algo_strings
>>>
>>> util/lar/lib.c:
>>> 	pull out function add_entry from add_files
>>>
>>> util/lar/lib.h:
>>> 	add prototypes for add_entry and lar_copy
>>> 	change prototype for create_lar (wonder why add_lar doesn't have
>>>       
>> one)
>>     
>>> 	fix whitespace
>>>
>>> util/lar/stream.c:
>>> 	pull out helper function file_in_list_where from file_in_list
>>> 	add lar_copy
>>>
>>> This is a test of the copy functionality of lar.  I'll use C++-style
>>>       
>> comments
>>     
>>>      // here's the original lar file
>>> [myles at orangutan svn]$ build/util/lar/lar -l build/coreboot.rom
>>>   normal/option_table (932 bytes @0x50);loadaddress 0x0 entry 0x0
>>>   normal/initram/segment0 (432 bytes @0x450);loadaddress 0x0 entry
>>>       
>> 0x0x42
>>     
>>>   normal/stage2/segment0 (191792 bytes, zeroes compressed to 1 bytes
>>> @0x650);loadaddress 0x0xa540 entry 0x0x2000
>>>   normal/stage2/segment1 (28172 bytes, lzma compressed to 15083 bytes
>>> @0x6b0);loadaddress 0x0x2000 entry 0x0x2000
>>>   normal/stage2/segment2 (5420 bytes, lzma compressed to 312 bytes
>>> @0x41f0);loadaddress 0x0x9000 entry 0x0x2000
>>>   bootblock (20480 bytes @0xfb000)
>>> Total size = 37688B 36KB (0x9338)
>>>
>>>      //create a new 256K lar file called bootblock.rom which has the
>>> same bootblock as coreboot.rom
>>> [myles at orangutan svn]$ build/util/lar/lar -c -s 256K bootblock.rom -L
>>> build/coreboot.rom
>>>
>>>      //create a new lar file with uncompressed normal/stage2
>>> [myles at orangutan svn]$ build/util/lar/lar -c -s 256K normal_stage2.rom
>>> -L build/coreboot.rom nocompress:normal/stage2:stage2
>>>
>>>       
>> Wait. You add a file with path "stage2" as "normal/stage2"? That's wrong.
>>     
>
> That's backward.  The first part of the argument is the original or file
> name.  I'm adding the entries from build/coreboot.rom named normal/stage2*
> and renaming them stage2*. 
>   

Thanks for clearing this up.


>>>      //create a new lar file with lzma compressed normal/init
>>> [myles at orangutan svn]$ build/util/lar/lar -c -s 256K normal_init.rom
>>> -L build/coreboot.rom -C lzma normal/init
>>>
>>>       
>> What is normal/init? That thing does not exist as path.
>>     
>
> I changed the matching to greedy matching in an earlier path, since it makes
> sense to get the whole payload at once.  In other words, when you extract
> normal/payload, you get normal/payload/segment0, normal/payload/segment1,
> etc.  I put this in there to make that clear.
>   

Sorry, had I known the greedy match didn't match at path boundaries, I
would have NACKed it. Having "normal/in" match "normal/initram/segment0"
as well as "normal/in" as well as "normal/interesting" is horrible.


>>>      //create a new lar file with normal/opt compressed the same as it
>>>       
>> was
>>     
>>> [myles at orangutan svn]$ build/util/lar/lar -c -s 256K normal_opt.rom -L
>>> build/coreboot.rom -C keep  normal/opt
>>>
>>>      //create a new 1M lar using the bootblock in bootblock.rom
>>> [myles at orangutan svn]$ build/util/lar/lar -c -s 1M new.rom -L
>>>       
>> bootblock.rom
>>     
>> How does LAR know that you want to copy the bootblock instead of an
>> empty operation?
>>     
>
> Since it is a create operation with a source lar, it copies the bootblock.
> It made sense to me that if you want a new lar starting with an old one, you
> were asking for the bootblock.
>   

That's an implicit undocumented action. It will trigger a lot of head
scratching in the future.


>>>      //add back in the components with the correct compression
>>> [myles at orangutan svn]$ build/util/lar/lar -a new.rom -L normal_opt.rom
>>> -C none normal/
>>> [myles at orangutan svn]$ build/util/lar/lar -a new.rom -L
>>> normal_init.rom -C none normal/init
>>>
>>>      //use the verbose switch to see what you're getting
>>> [myles at orangutan svn]$ build/util/lar/lar -av new.rom -L
>>> normal_stage2.rom -C lzma stage2:normal/stage2
>>>
>>>       
>> Here the stange stage2 mentioning happens again.
>>
>>     
>
> This is the opposite of what happened earlier.  Now I'm taking the files
> named stage2* and putting them into the new lar with base name normal/stage2
> so that the new lar will have the same format as build/coreboot.rom.
>   

OK.


>>> ... adding stage2:normal/stage2
>>> stage2:normal/stage2 (lzma) matches stage2/segment0 =>
>>>       
>> normal/stage2/segment0.
>>     
>>> stage2:normal/stage2 (lzma) matches stage2/segment1 =>
>>>       
>> normal/stage2/segment1.
>>     
>>> stage2:normal/stage2 (lzma) matches stage2/segment2 =>
>>>       
>> normal/stage2/segment2.
>>     
>>>      //diff the two files
>>> [myles at orangutan svn]$ diff new.rom build/coreboot.rom
>>>  // There's no difference
>>>
>>> I think it would be nice if lar would warn you (or stop you) when you
>>> try to add a duplicate entry.  Maybe that will be the next patch.
>>>
>>> Myles
>>>
>>> Signed-off-by: Myles Watson <mylesgw at gmail.com>
>>>       

One thing I didn't check yet: If you specify that compression of a file
should be changed, how does this affect the "zeroes" compression?

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/