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

[Manolo de Medici]

On Mon, Jan 22, 2024 at 6:04 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> (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.

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