Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
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
> 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
> > ---
> > 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
Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
On Mon, Jan 22, 2024 at 6:04 PM Peter Maydell wrote:
>
> (Cc'ing the block folks)
>
> On Thu, 18 Jan 2024 at 16:03, Manolo de Medici
> 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
> > ---
> > 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
Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
On Mon, Jan 22, 2024 at 9:23 PM Sergey Bugaev wrote: > call such a function. For example on GNU/Linux, remove(2) is a stub, (That was supposed to say revoke(2), of course.)
Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
Hello,
On Mon, Jan 22, 2024 at 8:05 PM Peter Maydell wrote:
>
> On Thu, 18 Jan 2024 at 16:03, Manolo de Medici
> 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.
Let me answer :)
glibc has this stubs mechanism: a function can be declared in the
system headers, but only implemented as a stub that always fails with
ENOSYS (or some such). You get a linker warning at link time if you
call such a function. For example on GNU/Linux, remove(2) is a stub,
and if I try to use it, the code does compile, but I get
/usr/bin/ld: /tmp/ccLCnRnW.o: in function `main':
demo.c:(.text+0xa): warning: revoke is not implemented and will always fail
during linking. This is done by embedding a
'.gnu.warning.function-name' section inside libc.so (try readelf
--wide --section-headers /lib64/libc.so.6 | grep warning). You can
also find the list of stubs in the gnu/stubs.h header, which contains
definitions like __stub_revoke.
Meson's has_function() knows about this mechanism, and returns false
if the function is declared, but is actually just a stub (by looking
for "__stub_{func}" being defined); autoconf does, too. But as the
prototype is still declared (and the function technically exists, too,
even if it's a stub), you'll get errors if you define the same
function incompatibly yourself.
Sergey
Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
(Cc'ing the block folks)
On Thu, 18 Jan 2024 at 16:03, Manolo de Medici 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
> ---
> 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
