Re: [PATCHv3 1/4] storage: initial support for linking with libgfapi

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Nov 11, 2013 at 09:19:28PM -0700, Eric Blake wrote:
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 72815f4..a90ee2b 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -555,6 +561,10 @@ BuildRequires: device-mapper-devel
>  BuildRequires: ceph-devel
>      %endif
>  %endif
> +%if %{with_storage_gluster}
> +BuildRequires: glusterfs-api-devel
> +BuildRequires: glusterfs-devel
> +%endif

This accepts any version of the packages

> diff --git a/m4/virt-gluster.m4 b/m4/virt-gluster.m4
> new file mode 100644
> index 0000000..4851e17
> --- /dev/null
> +++ b/m4/virt-gluster.m4

> +
> +AC_DEFUN([LIBVIRT_CHECK_GLUSTER],[
> +  LIBVIRT_CHECK_PKG([GLFS], [glusterfs-api], [3.0])
> +])

But this suggests only version >= 3.0 is acceptable. Should we make
the RPM dep explicit for this.

I think I'd prefer  s/GLFS/GLUSTERFS/ - its only used in a few
places, so being more verbose doesn't hurt us much.

> diff --git a/src/Makefile.am b/src/Makefile.am
> index ad39b74..be9978a 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1434,6 +1437,11 @@ if WITH_STORAGE_SHEEPDOG
>  libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_SHEEPDOG_SOURCES)
>  endif WITH_STORAGE_SHEEPDOG
> 
> +if WITH_STORAGE_GLUSTER
> +libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_GLUSTER_SOURCES)
> +libvirt_driver_storage_impl_la_LIBADD += $(GLFS_LIBS)

Hmm, I don't see CFLAGS ever mentioning  GLFS_CFLAGS which could
cause problems for anyone who has installed gluster outside of
the /usr hiearchy.

> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 975e662..8299f2a 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -55,7 +55,7 @@ VIR_ENUM_IMPL(virStoragePool,
>                VIR_STORAGE_POOL_LAST,
>                "dir", "fs", "netfs",
>                "logical", "disk", "iscsi",
> -              "scsi", "mpath", "rbd", "sheepdog")
> +              "scsi", "mpath", "rbd", "sheepdog", "gluster")

Hmm, well that sucks - in some places we use 'gluster' and others
we use 'glusterfs' :-( We've released code using both so we can't
standardize everywhere now.

So guess we should stick with what you have here.

> +    {.poolType = VIR_STORAGE_POOL_GLUSTER,
> +     .poolOptions = {
> +         .flags = (VIR_STORAGE_POOL_SOURCE_HOST |
> +                   VIR_STORAGE_POOL_SOURCE_NETWORK |
> +                   VIR_STORAGE_POOL_SOURCE_NAME),
> +     },

Are gluster volume names always wellformed paths ? If so is
it worth using SOURCE_PATH instad of SOURCE_NAME ? I don't
feel too strongly either way.

> +     .volOptions = {
> +         .defaultFormat = VIR_STORAGE_FILE_RAW,
> +         .formatToString = virStorageFileFormatTypeToString,
> +     }
> +    },
>      {.poolType = VIR_STORAGE_POOL_MPATH,
>       .volOptions = {
>           .formatToString = virStoragePoolFormatDiskTypeToString,

ACK if you fix the nitpicks.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]