Re: [PATCH] LZMA support in i386-pc kernel

[Robert Millan]

On Wed, Jul 02, 2008 at 11:11:28PM +0800, Bean wrote:
> >
> > Lenny's most likely going to ship with lzma 4.43.  Is that good?
> 
> The c encoder first appears in 4.58 beta. Previous version of lzma
> only have cpp version.

That's too bad, I guess it'll have to be for lenny+1.

> >> +#define FIXED_PROPS
> >> [...]
> >> +#ifdef FIXED_PROPS
> >
> > What's this option for?  Some comment would be nice.
> 
> lzma have three properties that control its behavior, lc, lp and pb.
> We can use these values as variable or constant. The default value, 3,
> 0, 2 is nice in most case, we hardly need to change them. So I use it
> as constant to save some extra space.

Could you make a comment out of this? :-)

> >> +#if 0
> >> +
> >> +DbgOut:
> >
> > This is just for debugging, right?  Maybe "#ifdef DEBUG" or so would be
> > better, to make it clear.
> 
> Yes, this is used in debugging. Now it's working, it's not needed
> anymore. I think it's better to keep it disable like this. The kernel
> raw space is critical, if we happen to define DEBUG, the decoder code
> would expand and kernel.img would fail to compile any more.

Ok.

> >> +#ifndef ASM_FILE
> >> +     xorl    %eax, %eax
> >> +#endif
> >
> > Why?  I thought ASM_FILE always ought to be defined for asm files in GRUB.
> 
> This is also used in debugging. Previously, I link it with c program
> to check the decoder, which require to keep register like %esi, %edi,
> %ebx. But inside grub, there is no need to keep them. I use ASM_FILE
> to distinguish between these two conditions.

Ok.  If you intend to keep it, please add a note explaining why; it
looks a bit puzzling without it.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)