[illumos-Developer] webrev: 278 get rid zfs of python and pyzfs dependencies

[Garrett D'Amore]

Jerry, thanks for your feedback!  And glad to see you've landed at
Joyent.

I'll make sure the engineer gets these issues resolved before
integration.

	- Garrett

On Mon, 2010-11-15 at 17:07 -0700, Jerry Jelinek wrote:
> > http://cr.illumos.org/view/jpj55p64/
> > 
> > This work was done by another Nexentian. The goal is to eliminate
> the
> > use of Python from zfs, which was "split" into having some
> percentage of
> > its code written in Python and the rest in C. (Fixing this is a step
> > along the road towards making python installation "optional" at
> Runtime.
> > The other dependencies are beadm and IPS. We're working on both of
> > those as well, and not all distros will use IPS anyway.)
> >
> > Please provide feedback. The timer on this expires in one week.
> 
> This looks good, thanks a lot for doing this. I have a few comments.
> 
> Did you ever run lint on this code? A bunch of my comments, plus some
> other issues, such as casting problems, should have been caught by
> lint.
> I didn't call out the casting issues.
> 
> usr/src/cmd/zfs/zfs_main.c
> 
> 1914,etc. Missing the error return check from nvlist_lookup calls in
> this
> function. Without that, it looks like the code could reuse
> values from previous iterations of the loop.
> 
> 2121, 2122 Variables unused
> 
> 2238,2239, 2245, 2246 Can you indent using the C style guidelines
> 
> 2263 Instead of '4', can you #define a constant up near line 2228
> which
> is closer to the array being indexed.
> 
> 3832 appears to be unused
> 
> 3920, 3921, 3923, appear to be unused
> 
> 3980 This variable is always set to be true so the conditional is
> unnecessary
> 
> 4023 memory leak, overwrites malloc from 3982 without first freeing
> 
> 4061 memory leak, return without freeing from alloc at 4046
> 
> 4077, etc all of the cases in this switch statement are missing a
> break
> so they all fall through to the default.
> 
> 4351, 4352 missing check for out of memory, also err set but never
> used
> 
> 4424, etc, can you use snprintf in this funtion to avoid buffer
> overflow
> issues
> 
> 4472 missing check for out of memory, also err set but never used
> 
> 4596 this use of strnlen could be simplified to just check the
> first element in the string for \0
> 
> 4704 could use the return value from 4700 instead of recomupting the 
> strlen
> 
> usr/src/lib/libzfs/common/libzfs_dataset.c
> 
> 4052 I think it is incorrect to print an error when you've chosen
> a too small buffer and are looping to retry at line 4055
> 
> 4105 this loop appears to be a no-op
> 
> 4186 I think it is incorrect to print an error when you've chosen
> a too small buffer and are looping to retry at line 4190
> 
> Thanks,
> Jerry 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer