Re: [libvirt] [PATCH 1/2] storage: zfs: enable on Linux

On 01/25/2016 08:37 PM, Roman Bogorodskiy wrote: > John Ferlan wrote: > >> >> >> On 01/02/2016 09:25 PM, Roman Bogorodskiy wrote: >>> ZFS-on-Linux implementation of ZFS starting with version 0.6.4 >>> contains all the features we use, so there's no reason to limit >>> libvirt ZFS storage backend to FreeBSD only. >>> >>> There's still one difference between these implementations: ZFS on >>> FreeBSD requires to set 'volmode' option to 'dev' when creating >>> volumes, while ZFS-on-Linux does not need that. >>> >>> Handle it by checking if 'volmode' option is needed by parsing usage >>> information of the 'zfs get' command. >>> --- >>> configure.ac | 8 ------ >>> src/storage/storage_backend_zfs.c | 51 +++++++++++++++++++++++++++++++++++++-- >>> 2 files changed, 49 insertions(+), 10 deletions(-) >>> >> >> Caveat - I know very little about zfs, FreeBSD... But it's languishing >> so... > > Heh. Thanks for stepping in! :) > Sorry for the delay - backlog grows fast these days. >>> diff --git a/configure.ac b/configure.ac >>> index a566f5b..d768341 100644 >>> --- a/configure.ac >>> +++ b/configure.ac >>> @@ -2005,14 +2005,6 @@ if test "$with_storage_gluster" = "yes"; then >>> fi >>> AM_CONDITIONAL([WITH_STORAGE_GLUSTER], [test "$with_storage_gluster" = "yes"]) >>> >>> -if test "$with_storage_zfs" = "check"; then >>> - with_storage_zfs=$with_freebsd >>> -fi >>> - >>> -if test "$with_storage_zfs" = "yes" && test "$with_freebsd" = "no"; then >>> - AC_MSG_ERROR([The ZFS storage driver can be enabled on FreeBSD only.]) >>> -fi >>> - >>> if test "$with_storage_zfs" = "yes" || >>> test "$with_storage_zfs" = "check"; then >>> AC_PATH_PROG([ZFS], [zfs], [], [$PATH:/sbin:/usr/sbin]) >> >> So it would seem this hunk and patch 2 would be more related. That is >> the virsh WITH_STORAGE_ZFS. The order of the patches is up to you. > > Actually, these two patches are unrelated. I should have been added the > virsh part when I added ZFS driver in the first place, but I forgot > about it at that time. > > I remembered about it when I was testing my configure.ac changes on > Linux and saw that virsh -V output does not change. > It would seem this should be a separate patch then since it's not related to the 'volmode' support. The configure checks aren't my area of expertise, but as long as you split it out, ACK to that. Based on the explanation you provide - an ACK for the following change as well.