On Sun, 21 Oct 2007, Shawn Walker wrote: > 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?
It should, I'll change it. > 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. That does seem like overkill, I'm going to think about what's better here. > 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. Yep, I'm going to replace 8k with MAXBSIZE. > 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? The lofi_uncompress function is significantly smaller than the lofi_compress function and so is the cleanup required before returning. It just seemed like overkill to do the same thing 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. Yep, it is my intent to remove this prior to putback like the comment says. > 576 + * NB. Incase an error occurs during compression (above) > > What does NB mean here? It is meant to serve as a "Note" to the reader -- the note answers the question (if it occurs to you), "What happens if an error occurs during li->l_compress()?" > "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... Agreed. > > 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? Unfortunately, no. > 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? I couldn't think of another one. ZFS uses MAXPROPLEN for it's properties (of which, compression algorithm is one). MAXPROPLEN is effectively MAXPATHLEN. I agree with Casper though that MAXPATHLEN + 1 is incorrect, it should be MAXPATHLEN instead. I'll change the pre-existing fields like 'li_filename' also to MAXPATHLEN. Thanks for your review :) Alok
