Eric C. Taylor wrote:
> Here's what I have so far.
>
> - Eric
>
> usr/src/cmd/zfs/zfs_main.c
>
> 4124: shouldn't there be a "?" case in the getopt switch?
ACCEPT. Yes there should, however it was actually caught by the else
clause at 4263.
> 4270: I think you need to check for a NULL pointer here.
ACCEPT.
> superfluous new lines at 3355, 3422.
ACCEPT
> usr/src/cmd/zpool/zpool_main.c
>
> 1328: "depending of" -> "depending on"
ACCEPT
> 3526: Any reason to say "Initial?"
Guess not, it was just a hint that there may be future phases. Best
to drop "Initial" from the description.
> 3900: "*zpool_get_libzfs_cry(zhp) =" seems wrong. Probably
> have a zpool_set_libzfs_cry() function instead.
ACCEPT I've added a zpool_set_libzfs_cry() and removed the _get_ variant
as well as the uncalled libzfs_get_crypt.
I also added the corresponding zfs_set_libzfs_cry() and updated
zfs_main.c. As well as making use of {zfs,zpool}_set_libzfs_cry()
in libzfs_crypto.c functions.
I also fixed the set_callback_key_{load,unload} so that they will return
the error from zpool_{load,unload}_key. I don't know why we were
masking that way.
> usr/src/common/zfs/zfs_prop.c
>
> 215: Can't the keyscope property use ZFS_TYPE_DATASET here?
ACCEPT.
> usr/src/common/zfs/zpool_prop.c
>
> 22: The copyright is out of date here and in a couple of other files.
ACCEPT
> 115: There are three values listed, but the table has four.
The missing value is "defined". However defined will never be seen
by any thing looking at the property.
See the comment in zio_crypt.h where we define zfs_crypt_key_status_t.
* keystatus is partially persistent and partially temporary.
* The are two states that persist on disk undefined and defined.
* If the on disk state is defined we return the appropriate "in memory"
* state of available or unavailable depending on wither or not the
* key is in the keystore.
*
* Old pool versions and datasets with encryption=off always have
* a keystatus of undefined.
> usr/src/common/zfs/zprop_common.c
>
> 348: update comment (or why can't a binary property be fixed width?)
Because we don't know what its length is, just like with strings.
On the other hand they kind of are fixed length because zfs_prop_get()
doesn't actually allow getting a value for them so I guess they are
fixed at width 0.
I've changed PROP_TYPE_BINARY to be fixed.
> usr/src/lib/libzfs/Makefile.com:
>
> Why do you need OBJS_SHARED_UTS?
ACCEPT, we don't any more that is left over from before zcrypt_common.c
was split out from zio_crypt.c.
> usr/src/lib/libzfs/common/libzfs.h
>
> 165-174: structure prefixes would be good here
ACCEPT, using zc_
> usr/src/lib/libzfs/common/libzfs_crypto.c
>
> should use zfs_alloc instead of malloc/bzero
ACCEPT, I also changed many of these to use the new
{zfs,zpool}_set_libzfs_cry() function instead of poking
the struct directly.
> 202, 223: missing gettext() call
ACCEPT.
> 477: strcmp on a string returned by a getenv call (inside
> of a library) seems dicey
I does a bit but I believe this is safe. However since this
is only relevant to zpool (1M) because of the call to 'zpool key -l -a'
that is in svc:/system/device/local I've moved the skip logic
to zpool_main.c.
> 495: STDIN_FILENO instead of zero?
ACCEPT
> 484, 871 #defining Esomething to 255 would be better
No longer applicable given the move of the key load bypass
stuff to zpool_main.c.
> 556: should there be a "default" case with an assert?
ACCEPT
> 896: are there any currently other clients other than zpool?
> if not I'd avoid this
Yes, the Lockhart based GUI and another two know ones, one unbundled
from ON that already has libzfs contracts and another that will
be a follow on feature from the ZFS Crypto team.
By "this" I assume you mean the call to zpool_create_zvol_links()
We need to do that because the links for encrypted zvols shouldn't
have been created if the pool was imported with the key not present.
I also arranged for the links to be removed for encrypted zvols
when the key is removed.
> 793, 1579, 1669: any reason to bzero before the free?
Good security practice to zero key material as soon as possible
after you are done with it to reduce the amount of time it lives
in process memory.
> usr/src/lib/libzfs/common/libzfs_mount.c
>
> 1196: may as well use zfs_spa_version()
REJECT. I can't because we have a zpool_handle_t and zfs_spa_version()
takes a zfs_handle_t and spa_version() takes a spa_t.
> usr/src/lib/libzfs/common/libzfs_pool.c
>
> 510: superfluous "break;"
ACCEPT
> 628, 1012: superfluous blank lines
ACCEPT
> usr/src/lib/libzfs/common/libzfs_util.c
>
> 565, 571, 577: move the brackets to the next line
N/A as these don't exist anymore replaced with _set_
> It seens like these should all be of type zfs_crypt_t *
> rather than zfs_crypt_t **.
N/A as these don't exist anymore replaced with _set_
> 1187: this should be taken care of by zfs_prop_readonly.
It should be except that the binary properties aren't read-only.
This is here because there is no userland API to set a binary
property - since there is no consumer for it yet. I've rather
leave this check in place than replacing it with an ASSERT (which
is what I originally had in there).
> usr/src/lib/libzfs/common/mapfile-vers
>
> 37: is there anyone outside of libzfs who calls
> valid_set_keysource_change? If so, it should probably
> have some sort of zfs prefix on the name.
No there isn't so it has been removed from the mapfile.
> 45: is there anyone outside of libzfs who calls
> zfs_crypto_create?
Originally we thought there might be, there certainly isn't
just now so I've removed it from the mapfile.
> usr/src/uts/common/fs/zfs/sys/spa.h
>
> 137: diagram shows highest order byte being used, but the
> macro on lines 235-236 uses the lowest order byte.
Fixed the diagram, and the corresponding version in $SRC/grub/ too.
> usr/src/uts/common/fs/zfs/spa.c
>
> 1952: why did you need to move this above the spa_prop_validate code?
http://defect.opensolaris.org/bz/show_bug.cgi?id=1631
Maybe it doesn't need to be flipped any more I'll investigate.
> usr/src/uts/common/fs/zfs/zio.c
>
> 1590: lets -> let's
N/A After mega merge/rewrite with 6754011 (onnv_100) this code doesn't
exist anymore so the comment isn't there to be correct.
> 1611: comment says ENOENT but code says EAGAIN
Fixed comment.
> 1625: call to printf()
ACCEPT, This had already been replaced by a DTRACE static probe.
> usr/src/uts/common/fs/zfs/zio_crypt.c
>
> 694, 1031: you can just use kmem_alloc here
ACCEPT, and 731 too.
> 718: use KM_SLEEP instead of "0"
ACCEPT, thats what I get for reading nvlist_alloc(3nvpair) and reading
that it was reserved and should be 0 rather than reading
nvlist_alloc(9f) where it is a kmflag field.
> 777: spa_keystatus is called here and other places without the
> sk_lock being held.
ACCEPT, I've put the rw_enter(sk_lock, RW_READER) inside spa_keystatus.
spa_keystatus() is called from dmu_objset.c and spa.c as well as
zio_crypt.c functions.
> 1207: ever -> every
ACCEPT
--
Darren J Moffat
