On Mon, Jan 22, 2024 at 6:04 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> (Cc'ing the block folks)
>
> On Thu, 18 Jan 2024 at 16:03, Manolo de Medici <manolodemed...@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.demed...@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.
Ok
>
> > {
> > #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.
>
Ok
> > #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.
>
I think Sergey explained very well why the Hurd its this piece of code
> 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...
I'd love to remove the syscall wrapper if you give me the ok to do it
Thanks
