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

[Kevin Wolf]

Am 22.01.2024 um 18:04 hat Peter Maydell geschrieben:
> (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.

Yes, I think we didn't expect that HAVE_COPY_FILE_RANGE would not be
defined in some cases even if copy_file_range() exists in the libc.

> 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.

Yes, I think this should be #if defined(__linux__) &&
defined(__NR_copy_file_range).

> 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...

The kernel doesn't really matter here, but the libc. Apparently
copy_file_range() was added in glibc 2.27 in 2018. If we want to remove
the wrapper, we'd have to check if all currently supported distributions
have a new enough glibc.

Kevin