block/gluster.c | 15 +++++++++++++-- configure | 8 ++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
---
block/gluster.c | 15 +++++++++++++--
configure | 8 ++++++++
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 4adc1a875b..2474580ad6 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -996,6 +996,7 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
PreallocMode prealloc, Error **errp)
{
int64_t current_length;
+ int ret;
current_length = glfs_lseek(fd, 0, SEEK_END);
if (current_length < 0) {
@@ -1023,7 +1024,12 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
#endif /* CONFIG_GLUSTERFS_FALLOCATE */
#ifdef CONFIG_GLUSTERFS_ZEROFILL
case PREALLOC_MODE_FULL:
- if (glfs_ftruncate(fd, offset)) {
+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
+ ret = glfs_ftruncate(fd, offset);
+#else
+ ret = glfs_ftruncate(fd, offset, NULL, NULL);
+#endif
+ if (ret) {
error_setg_errno(errp, errno, "Could not resize file");
return -errno;
}
@@ -1034,7 +1040,12 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
break;
#endif /* CONFIG_GLUSTERFS_ZEROFILL */
case PREALLOC_MODE_OFF:
- if (glfs_ftruncate(fd, offset)) {
+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
+ ret = glfs_ftruncate(fd, offset);
+#else
+ ret = glfs_ftruncate(fd, offset, NULL, NULL);
+#endif
+ if (ret) {
error_setg_errno(errp, errno, "Could not resize file");
return -errno;
}
diff --git a/configure b/configure
index 0a19b033bc..69827b0098 100755
--- a/configure
+++ b/configure
@@ -429,6 +429,7 @@ glusterfs_xlator_opt="no"
glusterfs_discard="no"
glusterfs_fallocate="no"
glusterfs_zerofill="no"
+glusterfs_legacy_ftruncate="no"
gtk=""
gtkabi=""
gtk_gl="no"
@@ -3856,6 +3857,9 @@ if test "$glusterfs" != "no" ; then
glusterfs_fallocate="yes"
glusterfs_zerofill="yes"
fi
+ if ! $pkg_config --atleast-version=7.4 glusterfs-api; then
+ glusterfs_legacy_ftruncate="yes"
+ fi
else
if test "$glusterfs" = "yes" ; then
feature_not_found "GlusterFS backend support" \
@@ -6502,6 +6506,10 @@ if test "$glusterfs_zerofill" = "yes" ; then
echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
fi
+if test "$glusterfs_legacy_ftruncate" = "yes" ; then
+ echo "CONFIG_GLUSTERFS_LEGACY_FTRUNCATE=y" >> $config_host_mak
+fi
+
if test "$libssh2" = "yes" ; then
echo "CONFIG_LIBSSH2=m" >> $config_host_mak
echo "LIBSSH2_CFLAGS=$libssh2_cflags" >> $config_host_mak
--
2.14.3
This change looks good to me, but a commit message would have been
helpful. I suggest something like this:
Gluster 4.0 changed the signature of glfs_ftruncate(). The function
now has two additional arguments, namely prestat and poststat. These
provide not benefit for QEMU, so ignoring them and passing NULL makes
the gluster-block driver compile with the new Gluster version again.
And maybe add this too:
Glusters libgfapi uses symbol versioning and provides backwards
compatible functions. Binaries compiled against previous versions of
Gluster keep on functioning with the new library.
Thanks!
Reviewed-by: Niels de Vos <ndevos@redhat.com>
On Thu, Apr 12, 2018 at 04:46:14PM +0530, Prasanna Kumar Kalever wrote:
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---
> block/gluster.c | 15 +++++++++++++--
> configure | 8 ++++++++
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index 4adc1a875b..2474580ad6 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -996,6 +996,7 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
> PreallocMode prealloc, Error **errp)
> {
> int64_t current_length;
> + int ret;
>
> current_length = glfs_lseek(fd, 0, SEEK_END);
> if (current_length < 0) {
> @@ -1023,7 +1024,12 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
> #endif /* CONFIG_GLUSTERFS_FALLOCATE */
> #ifdef CONFIG_GLUSTERFS_ZEROFILL
> case PREALLOC_MODE_FULL:
> - if (glfs_ftruncate(fd, offset)) {
> +#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> + ret = glfs_ftruncate(fd, offset);
> +#else
> + ret = glfs_ftruncate(fd, offset, NULL, NULL);
> +#endif
> + if (ret) {
> error_setg_errno(errp, errno, "Could not resize file");
> return -errno;
> }
> @@ -1034,7 +1040,12 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
> break;
> #endif /* CONFIG_GLUSTERFS_ZEROFILL */
> case PREALLOC_MODE_OFF:
> - if (glfs_ftruncate(fd, offset)) {
> +#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> + ret = glfs_ftruncate(fd, offset);
> +#else
> + ret = glfs_ftruncate(fd, offset, NULL, NULL);
> +#endif
> + if (ret) {
> error_setg_errno(errp, errno, "Could not resize file");
> return -errno;
> }
> diff --git a/configure b/configure
> index 0a19b033bc..69827b0098 100755
> --- a/configure
> +++ b/configure
> @@ -429,6 +429,7 @@ glusterfs_xlator_opt="no"
> glusterfs_discard="no"
> glusterfs_fallocate="no"
> glusterfs_zerofill="no"
> +glusterfs_legacy_ftruncate="no"
> gtk=""
> gtkabi=""
> gtk_gl="no"
> @@ -3856,6 +3857,9 @@ if test "$glusterfs" != "no" ; then
> glusterfs_fallocate="yes"
> glusterfs_zerofill="yes"
> fi
> + if ! $pkg_config --atleast-version=7.4 glusterfs-api; then
> + glusterfs_legacy_ftruncate="yes"
> + fi
> else
> if test "$glusterfs" = "yes" ; then
> feature_not_found "GlusterFS backend support" \
> @@ -6502,6 +6506,10 @@ if test "$glusterfs_zerofill" = "yes" ; then
> echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
> fi
>
> +if test "$glusterfs_legacy_ftruncate" = "yes" ; then
> + echo "CONFIG_GLUSTERFS_LEGACY_FTRUNCATE=y" >> $config_host_mak
> +fi
> +
> if test "$libssh2" = "yes" ; then
> echo "CONFIG_LIBSSH2=m" >> $config_host_mak
> echo "LIBSSH2_CFLAGS=$libssh2_cflags" >> $config_host_mak
> --
> 2.14.3
>
n 04/12/2018 08:31 AM, Niels de Vos wrote: > This change looks good to me, but a commit message would have been > helpful. I suggest something like this: > > Gluster 4.0 changed the signature of glfs_ftruncate(). The function > now has two additional arguments, namely prestat and poststat. These > provide not benefit for QEMU, so ignoring them and passing NULL makes > the gluster-block driver compile with the new Gluster version again. > > And maybe add this too: > > Glusters libgfapi uses symbol versioning and provides backwards > compatible functions. Binaries compiled against previous versions of > Gluster keep on functioning with the new library. > >> @@ -3856,6 +3857,9 @@ if test "$glusterfs" != "no" ; then >> glusterfs_fallocate="yes" >> glusterfs_zerofill="yes" >> fi >> + if ! $pkg_config --atleast-version=7.4 glusterfs-api; then >> + glusterfs_legacy_ftruncate="yes" Also, version-based tests are lousy. Feature-based tests (does a call to glfs_ftruncate(0, 0) compile without error? then we are legacy) are less brittle, especially when features can be backported across versions. So this should be reworked to a compile check, rather than a version query. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Apr 12, 2018 at 11:21:42AM -0500, Eric Blake wrote: > n 04/12/2018 08:31 AM, Niels de Vos wrote: > > This change looks good to me, but a commit message would have been > > helpful. I suggest something like this: > > > > Gluster 4.0 changed the signature of glfs_ftruncate(). The function > > now has two additional arguments, namely prestat and poststat. These > > provide not benefit for QEMU, so ignoring them and passing NULL makes > > the gluster-block driver compile with the new Gluster version again. > > > > And maybe add this too: > > > > Glusters libgfapi uses symbol versioning and provides backwards > > compatible functions. Binaries compiled against previous versions of > > Gluster keep on functioning with the new library. > > > > >> @@ -3856,6 +3857,9 @@ if test "$glusterfs" != "no" ; then > >> glusterfs_fallocate="yes" > >> glusterfs_zerofill="yes" > >> fi > >> + if ! $pkg_config --atleast-version=7.4 glusterfs-api; then > >> + glusterfs_legacy_ftruncate="yes" > > Also, version-based tests are lousy. Feature-based tests (does a call > to glfs_ftruncate(0, 0) compile without error? then we are legacy) are > less brittle, especially when features can be backported across > versions. So this should be reworked to a compile check, rather than a > version query. In this case, the cange to glfs_ftruncate() will not be backported in the releases from the Gluster Community or any distributions that I am familiar with. I agree that a compile-test is safer and can understand that those are preferred over pkg-config version checks. Niels
From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
function that returns additional 'struct stat' structures to enable
advanced caching of attributes. This is useful for file servers, not so
much for QEMU. Never the less, the API has changed and needs to be
adopted.
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Signed-off-by: Niels de Vos <ndevos@redhat.com>
--
v2: do a compile check as suggested by Eric Blake
---
block/gluster.c | 15 +++++++++++++--
configure | 18 ++++++++++++++++++
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 4fd55a9cc5..d1c6f81f5c 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -997,6 +997,7 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
PreallocMode prealloc, Error **errp)
{
int64_t current_length;
+ int ret;
current_length = glfs_lseek(fd, 0, SEEK_END);
if (current_length < 0) {
@@ -1024,7 +1025,12 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
#endif /* CONFIG_GLUSTERFS_FALLOCATE */
#ifdef CONFIG_GLUSTERFS_ZEROFILL
case PREALLOC_MODE_FULL:
- if (glfs_ftruncate(fd, offset)) {
+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
+ ret = glfs_ftruncate(fd, offset);
+#else
+ ret = glfs_ftruncate(fd, offset, NULL, NULL);
+#endif
+ if (ret) {
error_setg_errno(errp, errno, "Could not resize file");
return -errno;
}
@@ -1035,7 +1041,12 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
break;
#endif /* CONFIG_GLUSTERFS_ZEROFILL */
case PREALLOC_MODE_OFF:
- if (glfs_ftruncate(fd, offset)) {
+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
+ ret = glfs_ftruncate(fd, offset);
+#else
+ ret = glfs_ftruncate(fd, offset, NULL, NULL);
+#endif
+ if (ret) {
error_setg_errno(errp, errno, "Could not resize file");
return -errno;
}
diff --git a/configure b/configure
index 2a7796ea80..f3c0918d6b 100755
--- a/configure
+++ b/configure
@@ -451,6 +451,7 @@ glusterfs_xlator_opt="no"
glusterfs_discard="no"
glusterfs_fallocate="no"
glusterfs_zerofill="no"
+glusterfs_legacy_ftruncate="no"
gtk=""
gtkabi=""
gtk_gl="no"
@@ -3947,6 +3948,19 @@ if test "$glusterfs" != "no" ; then
glusterfs_fallocate="yes"
glusterfs_zerofill="yes"
fi
+ cat > $TMPC << EOF
+#include <glusterfs/api/glfs.h>
+
+int
+main(void)
+{
+ /* new glfs_ftruncate() passes two additional args */
+ return glfs_ftruncate(NULL, 0 /*, NULL, NULL */);
+}
+EOF
+ if compile_prog "$glusterfs_cflags" "$glusterfs_libs" ; then
+ glusterfs_legacy_ftruncate="yes"
+ fi
else
if test "$glusterfs" = "yes" ; then
feature_not_found "GlusterFS backend support" \
@@ -6644,6 +6658,10 @@ if test "$glusterfs_zerofill" = "yes" ; then
echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
fi
+if test "$glusterfs_legacy_ftruncate" = "yes" ; then
+ echo "CONFIG_GLUSTERFS_LEGACY_FTRUNCATE=y" >> $config_host_mak
+fi
+
if test "$libssh2" = "yes" ; then
echo "CONFIG_LIBSSH2=m" >> $config_host_mak
echo "LIBSSH2_CFLAGS=$libssh2_cflags" >> $config_host_mak
--
2.17.1
On 07/26/2018 04:19 AM, Niels de Vos wrote:
> From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
When sending a v2, it's best to start a new thread instead of
in-reply-to v1, so that the automated tooling spots it easier.
Subject line is awkward, may I suggest:
block/gluster: Handle changed glfs_ftruncate signature
>
> New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
> function that returns additional 'struct stat' structures to enable
> advanced caching of attributes. This is useful for file servers, not so
> much for QEMU. Never the less, the API has changed and needs to be
s/Never the less/Nevertheless/
> adopted.
>
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> Signed-off-by: Niels de Vos <ndevos@redhat.com>
>
> --
> v2: do a compile check as suggested by Eric Blake
> ---
> block/gluster.c | 15 +++++++++++++--
> configure | 18 ++++++++++++++++++
> 2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index 4fd55a9cc5..d1c6f81f5c 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -997,6 +997,7 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
> PreallocMode prealloc, Error **errp)
> {
> int64_t current_length;
> + int ret;
>
> current_length = glfs_lseek(fd, 0, SEEK_END);
> if (current_length < 0) {
> @@ -1024,7 +1025,12 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
> #endif /* CONFIG_GLUSTERFS_FALLOCATE */
> #ifdef CONFIG_GLUSTERFS_ZEROFILL
> case PREALLOC_MODE_FULL:
> - if (glfs_ftruncate(fd, offset)) {
> +#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> + ret = glfs_ftruncate(fd, offset);
> +#else
> + ret = glfs_ftruncate(fd, offset, NULL, NULL);
> +#endif
I'm personally a fan of minimizing in-function #ifdef, where it is easy.
This could be done by a top-level:
#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
# define glfs_ftruncate(fd, offs, _1, _2) glfs_ftruncate(fd, offs)
#endif
then just using glfs_ftruncate(fd, offs, NULL, NULL) unconditionally in
the rest of the file.
At any rate, the configure test looks a lot better than the v1 patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.