2009/3/2 Alok Aggarwal <Alok.Aggarwal at sun.com>:
>
> On Mon, 2 Mar 2009, Juergen Keil wrote:
>
>> usr/src/cmd/lofiadm/main.c lines 146 - 156:
>>
>> A usage function was added between the compress2p variable and the
>> gzip_compress function. The only use of compress2p is inside
>> gzip_compress(). Move the static variable compress2p inside
>> gzip_compress().
>>
>>
>> usr/src/cmd/lofiadm/main.c lines 186, 192, 206:
>>
>> Style function definition: move function identifier to column 0
>>
>> static void *
>> SzAlloc(void *p, size_t size)
>>
>> static void
>> SzFree(void *p, void *address, size_t size)
>>
>> static int
>> lzma_compress(void *src, size_t srclen, void *dst,
>> size_t *dstlen, int level)
>
> Changed all of the above.
lofiadm/main.c line 157:
The local variable compres2p in function gzip_compress() / lofiadm/main.c
should remain a static variable, so that openlib() and dlsym() is used
exactly once.
lofiadm/main.c line 184, 185:
The '*' for the void * return type of SzAlloc function should be moved
up one line
(I think the idea is that a grep for "^SzAlloc" should find the
function's definition):
static void *
SzAlloc(..)
Same in lofi.c, lines 199, 200
>> usr/src/cmd/lofiadm/main.c lines 227 .. 231
>>
>> Why is the space available in the destination buffer for compressed data
>> added to the compressed output? The information seems to be unused.
>> And this seems to waste 8 bytes for every compressed segment. When
>> cleaning this, all defines for LZMA_SIZE_OFFSET should be removed from
>> lofiadm.c, lofi.c; and LZMA_HEADER_SIZE == LZMA_PROPS_SIZE.
>
> This is picked directly from the LZMA SDK. The LZMA
> properties get stored in the first 8 bytes of every
> compressed segment.
8 bytes? LZMA_PROPS_SIZE is defined as 5:
% grep LZMA_PROPS_SIZE usr/src/common/lzma/*.h
usr/src/common/lzma/LzmaDec.h:#define LZMA_PROPS_SIZE 5
usr/src/common/lzma/LzmaEnc.h:#define LZMA_PROPS_SIZE 5
> (though admittedly it seems a little
> odd for LzmaEncode to take a pointer to the compressed
> data and not the start of the header)
>
> I've gotten rid of LZMA_SIZE_OFFSET.
The LZMA properties are a 5 bytes header, followed by an 8 byte
compressed buffer length, followed by the lzma compressed data.
The 8 byte compressed buffer length between the lzma properties
and the lzma compressed data confuses me. It was inserted by this
piece of code it lofiadm/main.c:
dstp = (Byte *)dst;
t = *dstlen;
for (i = 0; i < 8; i++, t >>= 8) {
dstp[LZMA_SIZE_OFFSET + i] = (Byte)t;
}
Is this length (*dstlen) needed? I don't think we need this 8-byte
length field; the lzma compressed data should immediately
follow the 5 byte lzma props data.
Ok, changing this now would make new lzma compressed
files incompatible with the lzma compressed files that you
can find on opensolaris global ISOs. We would be changing
the lzma compressed lofi data format.
Btw. the 8-byte length field in the solaris.zlib image on
osol-0906-108-global-x86.iso are wrong; I would expect
to see the value 0x000000000001ffff, but I do find the value
0x0001ffff0001ffff ?? Since nobody is using this data,
it doesn't matter...
With a binary that I compiled from the lzma lofi patch the
length is correctly encoded as 0x000000000001ffff.
# mdb solaris.zlib
> 0?36BnXXX
0: 6c 7a 6d 61 0 0 0 0
0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0
0 0 0 0
200 a4470000 80100
> +?8B
0x30: 0 0 0 0 0 0 0
0 <<<<< 1st index entry
> +
0x38: 0 0 0 0 0 0 78
8c <<<<< 2nd index entry
> +
0x40: 0 0 0 0 0 0 f2
58 <<<<< 3rd
> +
0x48: 0 0 0 0 0 1 1d
51 <<<<< 4th
> 30+47a4*8=X
23d50
> 23d50?Bn5Bn8B
0x23d50: 1 <<< compressed segment flag
5d 0 0 80 0 <<< lzma properties
ff ff 1 0 ff ff 1
0 <<< max compression buffer space available
> 23d50+788c
0x2b5dc: 1 <<< compressed segment flag
5d 0 0 80 0 <<< lzma properties
ff ff 1 0 ff ff 1
0 <<< max compression buffer space available
> 23d50+f258
0x32fa8: 1 <<< compressed segment flag
5d 0 0 80 0 <<< lzma properties
ff ff 1 0 ff ff 1
0 <<< max compression buffer space available
> 23d50+11d51
0x35aa1: 1
5d 0 0 80 0
ff ff 1 0 ff ff 1 0
>> usr/src/uts/common/io/lofi.c lines 199, 205
>>
>> Style function definition: move function identifier to column 0
>>
>>
>>
>> usr/src/uts/common/io/lofi.c line 951
>>
>> Shouldn't this assert test for ASSERT(i < lsp->ls_comp_index_sz - 1) ?
>>
>>
>> usr/src/uts/common/io/lofi.c line 958
>>
>> The test for "(i == eblkno)" should be deleted.
>> Testing "(i == lsp->ls_comp_index_sz - 2)" should be sufficient.
>>
>> /*
>> * The last segment is special in that it is
>> * most likely not going to be the same
>> * (uncompressed) size as the other segments.
>> */
>> if ((i == eblkno) &&
>> (i == lsp->ls_comp_index_sz - 2)) {
>> seglen = lsp->ls_uncomp_last_seg_sz;
>> } else {
>> seglen = lsp->ls_uncomp_seg_sz;
>> }
>
> I've made all of the above changes and regenerated the
> webrev -
>
> http://cr.opensolaris.org/~aalok/lzma-lofi/
>
> Thanks for the review!
> Alok
>
