| Message ID | 20180412111614.9724-1-prasanna.kalever@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- block/gluster.c | 15 +++++++++++++-- configure | 8 ++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)