diff mbox series

[V3] xfsprogs: remove platform_zero_range wrapper

Message ID be7f0845-5d5f-4af5-9ca9-3e4370b47d97@sandeen.net (mailing list archive)
State Accepted, archived
Headers show
Series [V3] xfsprogs: remove platform_zero_range wrapper | expand

Commit Message

Eric Sandeen June 7, 2024, 3:24 p.m. UTC 
Now that the HAVE_FALLOCATE guard around including
<linux/falloc.h> in linux/xfs.h has been removed via
15fb447f ("configure: don't check for fallocate"),
bad things can happen because we reference fallocate in
<xfs/linux.h> without defining _GNU_SOURCE:

$ cat test.c
#include <xfs/linux.h>

int main(void)
{
	return 0;
}

$ gcc -o test test.c
In file included from test.c:1:
/usr/include/xfs/linux.h: In function ‘platform_zero_range’:
/usr/include/xfs/linux.h:186:15: error: implicit declaration of function ‘fallocate’ [-Wimplicit-function-declaration]
  186 |         ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
      |               ^~~~~~~~~

i.e. xfs/linux.h includes fcntl.h without _GNU_SOURCE, so we
don't get an fallocate prototype.

Rather than playing games with header files, just remove the
platform_zero_range() wrapper - we have only one platform, and
only one caller after all - and simply call fallocate directly
if we have the FALLOC_FL_ZERO_RANGE flag defined.

(LTP also runs into this sort of problem at configure time ...)

Darrick points out that this changes a public header, but
platform_zero_range() has only been exposed by default
(without the oddball / internal xfsprogs guard) for a couple
of xfsprogs releases, so it's quite unlikely that anyone is
using this oddball fallocate wrapper.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---

V2: remove error variable, add to commit msg
V3: Drop FALLOC_FL_ZERO_RANGE #ifdef per hch's suggestion and
    add his RVB from V2, with changes.

NOTE: compile tested only

Comments

Darrick J. Wong June 12, 2024, 6:28 p.m. UTC | #1 
On Fri, Jun 07, 2024 at 10:24:52AM -0500, Eric Sandeen wrote:
> Now that the HAVE_FALLOCATE guard around including
> <linux/falloc.h> in linux/xfs.h has been removed via
> 15fb447f ("configure: don't check for fallocate"),
> bad things can happen because we reference fallocate in
> <xfs/linux.h> without defining _GNU_SOURCE:
> 
> $ cat test.c
> #include <xfs/linux.h>
> 
> int main(void)
> {
> 	return 0;
> }
> 
> $ gcc -o test test.c
> In file included from test.c:1:
> /usr/include/xfs/linux.h: In function ‘platform_zero_range’:
> /usr/include/xfs/linux.h:186:15: error: implicit declaration of function ‘fallocate’ [-Wimplicit-function-declaration]
>   186 |         ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
>       |               ^~~~~~~~~
> 
> i.e. xfs/linux.h includes fcntl.h without _GNU_SOURCE, so we
> don't get an fallocate prototype.
> 
> Rather than playing games with header files, just remove the
> platform_zero_range() wrapper - we have only one platform, and
> only one caller after all - and simply call fallocate directly
> if we have the FALLOC_FL_ZERO_RANGE flag defined.
> 
> (LTP also runs into this sort of problem at configure time ...)
> 
> Darrick points out that this changes a public header, but
> platform_zero_range() has only been exposed by default
> (without the oddball / internal xfsprogs guard) for a couple
> of xfsprogs releases, so it's quite unlikely that anyone is
> using this oddball fallocate wrapper.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Ok I'm convinced
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> 
> V2: remove error variable, add to commit msg
> V3: Drop FALLOC_FL_ZERO_RANGE #ifdef per hch's suggestion and
>     add his RVB from V2, with changes.
> 
> NOTE: compile tested only
> 
> diff --git a/include/linux.h b/include/linux.h
> index 95a0deee..a13072d2 100644
> --- a/include/linux.h
> +++ b/include/linux.h
> @@ -174,24 +174,6 @@ static inline void platform_mntent_close(struct mntent_cursor * cursor)
>  	endmntent(cursor->mtabp);
>  }
>  
> -#if defined(FALLOC_FL_ZERO_RANGE)
> -static inline int
> -platform_zero_range(
> -	int		fd,
> -	xfs_off_t	start,
> -	size_t		len)
> -{
> -	int ret;
> -
> -	ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
> -	if (!ret)
> -		return 0;
> -	return -errno;
> -}
> -#else
> -#define platform_zero_range(fd, s, l)	(-EOPNOTSUPP)
> -#endif
> -
>  /*
>   * Use SIGKILL to simulate an immediate program crash, without a chance to run
>   * atexit handlers.
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 153007d5..b54505b5 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -73,7 +73,7 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
>  
>  	/* try to use special zeroing methods, fall back to writes if needed */
>  	len_bytes = LIBXFS_BBTOOFF64(len);
> -	error = platform_zero_range(fd, start_offset, len_bytes);
> +	error = fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes);
>  	if (!error) {
>  		xfs_buftarg_trip_write(btp);
>  		return 0;
> 
>
Petr Vorel June 19, 2024, 4:17 p.m. UTC | #2 
From: Eric Sandeen <sandeen@sandeen.net>

> Now that the HAVE_FALLOCATE guard around including
> <linux/falloc.h> in linux/xfs.h has been removed via
> 15fb447f ("configure: don't check for fallocate"),
> bad things can happen because we reference fallocate in
> <xfs/linux.h> without defining _GNU_SOURCE:

> $ cat test.c
> #include <xfs/linux.h>

> int main(void)
> {
> 	return 0;
> }

> $ gcc -o test test.c
> In file included from test.c:1:
> /usr/include/xfs/linux.h: In function ‘platform_zero_range’:
> /usr/include/xfs/linux.h:186:15: error: implicit declaration of function ‘fallocate’ [-Wimplicit-function-declaration]
>   186 |         ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
>       |               ^~~~~~~~~

> i.e. xfs/linux.h includes fcntl.h without _GNU_SOURCE, so we
> don't get an fallocate prototype.

> Rather than playing games with header files, just remove the
> platform_zero_range() wrapper - we have only one platform, and
> only one caller after all - and simply call fallocate directly
> if we have the FALLOC_FL_ZERO_RANGE flag defined.

> (LTP also runs into this sort of problem at configure time ...)

> Darrick points out that this changes a public header, but
> platform_zero_range() has only been exposed by default
> (without the oddball / internal xfsprogs guard) for a couple
> of xfsprogs releases, so it's quite unlikely that anyone is
> using this oddball fallocate wrapper.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Tested-by: Petr Vorel <pvorel@suse.cz>

I suppose this should be added
Fixes: 9d6023a8 ("libxfs: use FALLOC_FL_ZERO_RANGE in libxfs_device_zero")

Kind regards,
Petr

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---

> V2: remove error variable, add to commit msg
> V3: Drop FALLOC_FL_ZERO_RANGE #ifdef per hch's suggestion and
>     add his RVB from V2, with changes.

> NOTE: compile tested only

> diff --git a/include/linux.h b/include/linux.h
> index 95a0deee..a13072d2 100644
> --- a/include/linux.h
> +++ b/include/linux.h
> @@ -174,24 +174,6 @@ static inline void platform_mntent_close(struct mntent_cursor * cursor)
>  	endmntent(cursor->mtabp);
>  }

> -#if defined(FALLOC_FL_ZERO_RANGE)
> -static inline int
> -platform_zero_range(
> -	int		fd,
> -	xfs_off_t	start,
> -	size_t		len)
> -{
> -	int ret;
> -
> -	ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
> -	if (!ret)
> -		return 0;
> -	return -errno;
> -}
> -#else
> -#define platform_zero_range(fd, s, l)	(-EOPNOTSUPP)
> -#endif
> -
>  /*
>   * Use SIGKILL to simulate an immediate program crash, without a chance to run
>   * atexit handlers.
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 153007d5..b54505b5 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -73,7 +73,7 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)

>  	/* try to use special zeroing methods, fall back to writes if needed */
>  	len_bytes = LIBXFS_BBTOOFF64(len);
> -	error = platform_zero_range(fd, start_offset, len_bytes);
> +	error = fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes);
>  	if (!error) {
>  		xfs_buftarg_trip_write(btp);
>  		return 0;
diff mbox series

Patch

diff --git a/include/linux.h b/include/linux.h
index 95a0deee..a13072d2 100644
--- a/include/linux.h
+++ b/include/linux.h
@@ -174,24 +174,6 @@  static inline void platform_mntent_close(struct mntent_cursor * cursor)
 	endmntent(cursor->mtabp);
 }
 
-#if defined(FALLOC_FL_ZERO_RANGE)
-static inline int
-platform_zero_range(
-	int		fd,
-	xfs_off_t	start,
-	size_t		len)
-{
-	int ret;
-
-	ret = fallocate(fd, FALLOC_FL_ZERO_RANGE, start, len);
-	if (!ret)
-		return 0;
-	return -errno;
-}
-#else
-#define platform_zero_range(fd, s, l)	(-EOPNOTSUPP)
-#endif
-
 /*
  * Use SIGKILL to simulate an immediate program crash, without a chance to run
  * atexit handlers.
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 153007d5..b54505b5 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -73,7 +73,7 @@  libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
 
 	/* try to use special zeroing methods, fall back to writes if needed */
 	len_bytes = LIBXFS_BBTOOFF64(len);
-	error = platform_zero_range(fd, start_offset, len_bytes);
+	error = fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes);
 	if (!error) {
 		xfs_buftarg_trip_write(btp);
 		return 0;