[illumos-Developer] Webrev 247 (Partial Code)

[Alexander Stetsenko]

Hi Albert

Thank you a lot for your review and clarification, all your comments 
seems to be very reasonable.

On 10/21/2010 10:10 AM, Albert Lee wrote:
> Hi Alexander,
>
> On Tue, Oct 19, 2010 at 12:28 PM, Alexander Stetsenko<ams at nexenta.com>  wrote:
>    
>> This is webrev url:
>>
>> http://cr.illumos.org/view/ueymg8dy/
>> I`d like  to get some feedback on it.
>> This is the first code I wrote for illumos.
>> Currently zfs holds is functional on 100 percents.
>> zfs userspace lacks implementation for -i option (and more testing is still
>> required)
>>      
> The implementation looks good except for some minor issues, although I
> understand this is a work in progress. I notice you added an '-H'
> option to holds.
>    
I guess it will be helpful undocumented feature :-)
> Are the message strings the same as the existing implementation's
> (i.e. for translations)?
>    
It should be. I will check all this at final code polishing and testing.
> libzfs_dataset.c:
> ENOENT is a possible error code you should handle, as
> dsl_dataset_get_holds() calls dsl_dataset_hold().
> if (zhp == NULL) is probably not necessary - all of the functions
> assume the caller is using validated arguments.
> Seems to be more consistent to use the lowercase assert() macro.
>
> zfs_main.c:
> The holds_cbdata.cb_max_*len points should have a 'p' suffix or to
> differentiate them from us_cbdata.cb_max_*len values. It might be
> better for them to be the same types for consistency.
>    
Yes, I`m going to replace pointers by values in holds_cbdata.
> You should probably keep the macro and struct definitions near the
> beginning of the file.
> Declarations of stack variables are usually separate from non-static
> initialisers.
> The loop in print_holds() could use a single printf(), e.g.
>      char sep = scripted ? "\t" : " ";
>      printf("%-*s%c%-*s%c", nwidth, zname,
>          sep, tagwidth, tagname, sep;
>
> There are some unused variable declarations, like nicepropname in
> userspace_cb(), and weird changes to zfs_do_python() - I assume these
> are still being worked on.
> Any particular reason us_type2str() doesn't return a char * directly?
>
>    
>> I checked my changes with cstyle script. The posted code is almost fully
>> compliant with required c style, except this: "don't use boolean ! with
>> comparison functions". IMHO this is not very good style to compare result
>> with 0 :/. (but this is IMHO of course)
>>      
> The cstyle follows the written style guide which aims for
> maintainability rather than terseness, you can think of it as
> enforcing stronger type restrictions than C. For example, the string
> functions don't return boolean values, so it's against style to use
> boolean tests on those values. Doubly so for pointers.
>
>    
>>         for (; sortcol; sortcol = sortcol->sc_next) {
>>      
> Should be for (; sortcol != NULL; sortcol = sortcol->sc_next) {
>
>
> Binary/ternary operators should be separated by parentheses:
>    
>>                                  rc = rv32>  lv32 ? 1 : -1;
>>      
>                                   rc = (rv32>  lv32) ? 1 : -1;
>
>    
>> btw hg pbchk gives more code style issues than cstyle script.
>>
>> I need to realize allow/unallow command, so my work with zfs is still not
>> complete. I`ll polish code before final push to repository.
>>
>> Are there some documents about osol coding style?
>>
>>      
> Yes, the style guide used for ON is
> http://hub.opensolaris.org/bin/download/Community+Group+on/WebHome/cstyle.ms.pdf
>
> -Albert
>