* Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
2024-01-22 17:04 ` Peter Maydell
@ 2024-01-22 18:23 ` Sergey Bugaev
2024-01-22 18:26 ` Sergey Bugaev
2024-01-23 15:19 ` Manolo de Medici
2024-01-23 17:19 ` Kevin Wolf
2 siblings, 1 reply; 6+ messages in thread
From: Sergey Bugaev @ 2024-01-22 18:23 UTC (permalink / raw)
To: Peter Maydell, Manolo de Medici
Cc: qemu-devel, bug-hurd, Qemu-block, Kevin Wolf
Hello,
On Mon, Jan 22, 2024 at 8:05 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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.
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
2024-01-22 18:23 ` Sergey Bugaev
@ 2024-01-22 18:26 ` Sergey Bugaev
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Bugaev @ 2024-01-22 18:26 UTC (permalink / raw)
To: Peter Maydell, Manolo de Medici
Cc: qemu-devel, bug-hurd, Qemu-block, Kevin Wolf
On Mon, Jan 22, 2024 at 9:23 PM Sergey Bugaev <bugaevc@gmail.com> wrote:
> call such a function. For example on GNU/Linux, remove(2) is a stub,
(That was supposed to say revoke(2), of course.)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
2024-01-22 17:04 ` Peter Maydell
2024-01-22 18:23 ` Sergey Bugaev
@ 2024-01-23 15:19 ` Manolo de Medici
2024-01-23 17:19 ` Kevin Wolf
2 siblings, 0 replies; 6+ messages in thread
From: Manolo de Medici @ 2024-01-23 15:19 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, bug-hurd, Qemu-block, Kevin Wolf
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/4] Avoid conflicting types for 'copy_file_range'
2024-01-22 17:04 ` Peter Maydell
2024-01-22 18:23 ` Sergey Bugaev
2024-01-23 15:19 ` Manolo de Medici
@ 2024-01-23 17:19 ` Kevin Wolf
2 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2024-01-23 17:19 UTC (permalink / raw)
To: Peter Maydell; +Cc: Manolo de Medici, qemu-devel, bug-hurd, Qemu-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread