[illumos-Developer] Webrev 247 (Partial Code)

[Albert Lee]

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.
Are the message strings the same as the existing implementation's
(i.e. for translations)?

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.
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