On 17/10/2007, Alok Aggarwal <Alok.Aggarwal at sun.com> wrote: > I'd like to request codereview comments for > changes to lofi(7D) to add compression. The > webrev can be found here - > > http://cr.opensolaris.org/~aalok/clofi/
Overall, I like the changes. I have some nitpicks though. >From >http://cr.opensolaris.org/~aalok/clofi/usr/src/cmd/lofiadm/main.c.wdiff.html: 334 + char buf[8192]; Should 8192 be MAXBSIZE? 335 + char devicename[32]; It seems like 5 + sizeof(LOFI_BLOCK_NAME) + 1 + (string sizeof(MAXMIN)) + 1 might be better than 32? Perhaps that's overkill, but since the size of a device minor is different depending on 32-bit or 64-bit environment, etc. I don't like picking an arbitrary number of bytes to hold the device name. 410 + /* now read from the device in 8k chunks */ Not strictly correct; 8KiB ;) However, this could end up being MAXBSIZE instead based on my comment for line 334. As an aside, in lofi_compress, you took the approach of having a "cleanup:" block that you just did a "goto cleanup" for. Why didn't you do the same for lofi_uncompress? 560 + /* XXX remove before putback */ 561 +#if 0 562 + error = compress2(compressed_seg + SEGHDR, 563 + (ulong_t *)&real_segsize, uncompressed_seg, 564 + rbytes, 9); 565 +#endif I assume these are known. 576 + * NB. Incase an error occurs during compression (above) What does NB mean here? "Incase" -> "In case" 591 + * set the first byte or the SEGHDR to 592 + * indicate if it's compressed or not Sometimes you use punctuation; sometimes you do not. Sometimes you capitalise the first word of your sentences; sometimes you do not. Consistency is appreciated... 620 + * now write the compressed data alongwith the "alongwith" -> "along with" >From >http://cr.opensolaris.org/~aalok/clofi/usr/src/uts/common/io/lofi.c.wdiff.html: 500 + /* 501 + * Compute starting and ending compressed segment numbers 502 + * We use only bitwise operations avoiding division and 503 + * modulus because we enforce the compression segment size 504 + * to a power of 2 505 + */ Here again, you sometimes use punctuation; sometimes you do not. 1047 + /* XXX there seems to be extra rounding up being done here -- why? */ 1048 + /* simpler to just use roundup() twice instead */ Was this mystery ever solved? >From >http://cr.opensolaris.org/~aalok/clofi/usr/src/uts/common/sys/lofi.h.wdiff.html: 113 + char li_algorithm[MAXPATHLEN + 1]; MAXPATHLEN doesn't seem like the right length for the algorithm name. Is there a better one? Ta, -- Shawn Walker, Software and Systems Analyst binarycrusader at gmail.com - http://binarycrusader.blogspot.com/ "Beware of bugs in the above code; I have only proved it correct, not tried it. " --Donald Knuth
