Hi Richard,

On Fri, 16 Sep 2016 at 04:32:24 -0500, Richard Laager wrote:
> The attached patch adds ZFS support to cryptsetup.

Thanks the patch.  Unfortunately I can't test it easily, and I know
nothing about ZFS, but I'll try to review your patch anyway.

> +     zfs list -H -o name,canmount,mountpoint 2>/dev/null | \

Looks like there is support for ZFS filesystems in the /etc/fstab
<https://wiki.freebsd.org/RootOnZFS#Alternate_.2Fetc.2Ffstab>.  Since
it's what other FS are using, I think that's what we should use for ZFS
too.  Don't the ‘name’ and ‘mountpoint’ columns of your ‘zfs list’
command respectively correspond to the first and second columns of
fstab(5)?

> +                     for dev in $(zpool status -P "${name%%/*}" 2>/dev/null 
> | awk '($1 ~ /\//) {print $1}'); do

This looks rather brittle, as the ‘status’ command seems to be intended
for humans not parsers.  Isn't there a more robust way to retrieve the
underlying device list of a ZFS pool?  (FWIW, our current solution for
BTRFS volumes isn't satisfactory either.)

Cheers,
-- 
Guilhem.

Attachment: signature.asc
Description: PGP signature

Reply via email to