Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'

[Peter Maydell]

(Cc'ing the block folks)

On Thu, 18 Jan 2024 at 16:03, Manolo de Medici <manolodemedici@gmail.com> wrote:
>
> Compilation fails on systems where copy_file_range is already defined as a
> stub.

What do you mean by "stub" here ? If the system headers define
a prototype for the function, I would have expected the
meson check to set HAVE_COPY_FILE_RANGE, and then this
function doesn't get defined at all. That is, the prototype
mismatch shouldn't matter because if the prototype exists we
use the libc function, and if it doesn't then we use our version.

> The prototype of copy_file_range in glibc returns an ssize_t, not an off_t.
>
> The function currently only exists on linux and freebsd, and in both cases
> the return type is ssize_t
>
> Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> ---
>  block/file-posix.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..f744b35642 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2000,12 +2000,13 @@ static int handle_aiocb_write_zeroes_unmap(void 
> *opaque)
>  }
>
>  #ifndef HAVE_COPY_FILE_RANGE
> -static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
> -                             off_t *out_off, size_t len, unsigned int flags)
> +ssize_t copy_file_range (int infd, off_t *pinoff,
> +                         int outfd, off_t *poutoff,
> +                         size_t length, unsigned int flags)

No space after "copy_file_range". No need to rename all the
arguments either.

>  {
>  #ifdef __NR_copy_file_range
> -    return syscall(__NR_copy_file_range, in_fd, in_off, out_fd,
> -                   out_off, len, flags);
> +    return (ssize_t)syscall(__NR_copy_file_range, infd, pinoff, outfd,
> +                            poutoff, length, flags);

Don't need a cast here, because returning the value will
automatically cast it to the right thing.

>  #else
>      errno = ENOSYS;
>      return -1;

These changes aren't wrong, but as noted above I'm surprised that
the Hurd gets into this code at all.

Note for Kevin: shouldn't this direct use of syscall() have
some sort of OS-specific guard on it? There's nothing that
says that a non-Linux host OS has to have the same set of
arguments to an __NR_copy_file_range syscall. If this
fallback is a Linux-ism we should guard it appropriately.

For that matter, at what point can we just remove the fallback
entirely? copy_file_range() went into Linux in 4.5, apparently,
which is now nearly 8 years old. Maybe all our supported
hosts now have a new enough kernel and we can drop this
somewhat ugly syscall() wrapper...

thanks
-- PMM