On Sun, Nov 1, 2015 at 4:36 PM, Alexandre Lécuyer < [email protected]> wrote:
> Thank you for reviewing the patch. Sorry about the style and indentation > issues, I have corrected the ones you mentionned and ran cstyle on all > changed files. > > The webrev is updated, and I have uploaded the diff on the illumos review > board, which should make it easier to review :) > https://www.illumos.org/rb/r/111/ > > Comments about other changes below : > > Le 30/10/2015 23:53, Matthew Ahrens a écrit : > >> Aaah webrevs. Makes even github code review seem sophisticated and >> painless :-) >> >> zfs_prop.c: I don't think the new prop is applicable to bookmarks. >> > Right, I have removed it. > > >> zfs_create(), libzfs_changelist.c: Indentation looks wrong. Make sure >> you are using tabs, and continuation lines are indented with tabs + 4 >> spaces. >> >> zfs_create(): what error does lzc_create() if you try to create a >> filesystem that already exists? >> > lzc_create() is not called in this case, we stop at line 3191, > zfs_dataset_exists() is true and an error is returned ("dataset already > exists") OK, but: A. What if the dataset doesn't exist when zfs_dataset_exists() is called, and then another thread creates it before the lzc/ioctl is made? B. The lzc_* interfaces should provide a good interface for all consumers, not just this one. If we use the same error code for 2 totally different types of error, it's hard to use. > > > >> dsl_dataset.c: >> 3510: put a blank line after the end of the function, before the typedef.. >> 3514: only need one blank line here >> 3566: only one blank line >> 3567: "int" should be on its own line >> 3576: this should be ZFS_SPACE_CHECK_RESERVED. See the comments in >> dsl_synctask.h. >> > Done > > 3546: I assume it's OK to not manipulate the unique avl because we know >> this is not mounted. Can you add an assertion to that effect? >> > On line 3552 there is : > VERIFY0(dsl_dataset_hold(dp, ddsfg->ddsfg_name, FTAG, &ds)); > Is this OK ? Holding doesn't ensure that it is not mounted. (Checking if it's owned would) Actually, even not being mounted is not strong enough. It will be in the avl tree if the dataset_t exists at all (see where we call unique_insert()). You might need to manipulate the AVL after all, unless you can ensure that the dataset_t doesn't exist. --matt > > > the _check and _sync funcs do not seem to be used outside this file, >> they should be declared "static". >> > Done > > >> lines 3517 - 3563 are indented incorrectly. >> >> additional cstyle errors on lines 3532, 3539, 3540, 3534 >> >> dsl_dataset.h: >> the _check and _sync funcs do not seem to be used outside dsl_datset.c, >> they should not be declared here. >> > Done > > >> unique.c: >> changes to this file are incorrectly indented >> additional cstyle error line 113-114. >> 110: prefer to explicitly convert pointer to boolean with "!= NULL", and >> number to boolean with "!= 0". >> is line 110 <=80 columns? >> this might be easier to read if you do: >> boolean_t rv = (value & ... == 0 && avl_find(...) == NULL); >> mutex_exit(...); >> return (rv); >> > Changed as suggested > > >> zfs_ioctl.c: >> changes to this file are incorrectly indented >> >> zfs.h: >> prefer to add new props to end of the enum, to maintain user/kernel >> compatibility for platforms that do not keep them in sync (linux, freebsd) >> > Done > > Thanks, > -- > alex > >> >> >> Let me know if any of the cstyle errors are not obvious (or look at the >> rest of the file and see how it's done). The cstyle tool can catch some >> of these. >> >> --matt >> >> >> >> >> >> On Wed, Oct 28, 2015 at 6:10 AM, Alexandre Lécuyer >> <[email protected] <mailto:[email protected]>> wrote: >> >> Hello, >> >> Here's a patch which allows modification of the fsid_guid of a >> dataset : >> zfs create -o fsid_guid=xxx tank/test ; or zfs set fsid_guid=xxx >> tank/test >> >> This is useful if you need to migrate NFS clients from a target >> system to another, without having to umount/remount all clients. >> >> I have made changes that were suggested at the hackathon last week, >> thanks to all for your suggestions. >> If this is of interest to others, would you please review it ? I >> have uploaded a webrev here : >> >> http://cr.illumos.org/~webrev/alecuyer/6333/ >> >> thanks, >> -- >> alex >> >> >> ------------------------------------------- >> illumos-developer >> Archives: https://www.listbox.com/member/archive/182179/=now >> RSS Feed: >> https://www.listbox.com/member/archive/rss/182179/27166212-aafb8f50 >> Modify Your Subscription: >> >> https://www.listbox.com/member/?member_id=27166212&id_secret=27166212-5bda6a50 >> Powered by Listbox: http://www.listbox.com >> >> >>
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
