Re: [PATCH] xfsprogs: remove platform_zero_range wrapper

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jun 05, 2024 at 10:38:20PM -0500, Eric Sandeen wrote:
> Now that the 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 ...)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> 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

Technically speaking, this is an abi change in the xfs library headers
so you can't just yank this without a deprecation period.  That said,
debian codesearch doesn't show any users ... so if there's nothing in
RHEL/Fedora then perhaps it's ok to do that?

Fedora magazine pointed me at "sourcegraph" so I tried:
https://sourcegraph.com/search?q=context:global+repo:%5Esrc.fedoraproject.org/+platform_zero_range&patternType=regexp&sm=0

It shows no callers, but it doesn't show the definition either.

> -
>  /*
>   * 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..e5b6b5de 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -67,17 +67,19 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
>  	ssize_t		zsize, bytes;
>  	size_t		len_bytes;
>  	char		*z;
> -	int		error;
> +	int		error = 0;

Is this declaration going to cause build warnings about unused variables
if built on a system that doesn't have FALLOC_FL_ZERO_RANGE?

(Maybe we don't care?)

--D

>  
>  	start_offset = LIBXFS_BBTOOFF64(start);
>  
>  	/* 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);
> +#if defined(FALLOC_FL_ZERO_RANGE)
> +	error = fallocate(fd, FALLOC_FL_ZERO_RANGE, start_offset, len_bytes);
>  	if (!error) {
>  		xfs_buftarg_trip_write(btp);
>  		return 0;
>  	}
> +#endif
>  
>  	zsize = min(BDSTRAT_SIZE, BBTOB(len));
>  	if ((z = memalign(libxfs_device_alignment(), zsize)) == NULL) {
> 
>