[PATCH 2/4] Avoid multiple definitions of copy_file_range

[PATCH 2/4] Avoid multiple definitions of copy_file_range

[Manolo de Medici]

It's already defined as a stub on the GNU Hurd.

Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>

diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..05426abb7d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1999,7 +1999,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
     return handle_aiocb_write_zeroes(aiocb);
 }

-#ifndef HAVE_COPY_FILE_RANGE
+#if !defined(HAVE_COPY_FILE_RANGE) && !defined(__GNU__)
 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)
 {
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..05426abb7d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1999,7 +1999,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
     return handle_aiocb_write_zeroes(aiocb);
 }

-#ifndef HAVE_COPY_FILE_RANGE
+#if !defined(HAVE_COPY_FILE_RANGE) && !defined(__GNU__)
 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)
 {
--
2.43.0

Re: [PATCH 2/4] Avoid multiple definitions of copy_file_range

[Philippe Mathieu-Daudé]

Hi Manolo,

On 17/1/24 13:31, Manolo de Medici wrote:
> It's already defined as a stub on the GNU Hurd.

Meson checks for this function and defines
HAVE_COPY_FILE_RANGE if available, see in meson.build:

   config_host_data.set('HAVE_COPY_FILE_RANGE',
                        cc.has_function('copy_file_range'))

Maybe some header is missing in "osdep.h" for GNU Hurd?

> Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..05426abb7d 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1999,7 +1999,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
>       return handle_aiocb_write_zeroes(aiocb);
>   }
> 
> -#ifndef HAVE_COPY_FILE_RANGE
> +#if !defined(HAVE_COPY_FILE_RANGE) && !defined(__GNU__)
>   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)
>   {
> ---
>   block/file-posix.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35684f7e21..05426abb7d 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1999,7 +1999,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
>       return handle_aiocb_write_zeroes(aiocb);
>   }
> 
> -#ifndef HAVE_COPY_FILE_RANGE
> +#if !defined(HAVE_COPY_FILE_RANGE) && !defined(__GNU__)
>   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)
>   {
> --
> 2.43.0
> 

Re: [PATCH 2/4] Avoid multiple definitions of copy_file_range

[Manolo de Medici]

Hello Philippe,
thank you for the feedback, I've checked that. The problem is that the
Hurd fails that test due to the following:

        #if defined __stub_copy_file_range || defined __stub___copy_file_range
        fail fail fail this function is not going to work
        #endifefines the stub __copy_file_ran

rightfully so I'd say, because copy_file_range is just a stub on the Hurd.

As such, we really need to exclude the code that defines the stub in
qemu on the Hurd.

On Wed, Jan 17, 2024 at 2:56 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Manolo,
>
> On 17/1/24 13:31, Manolo de Medici wrote:
> > It's already defined as a stub on the GNU Hurd.
>
> Meson checks for this function and defines
> HAVE_COPY_FILE_RANGE if available, see in meson.build:
>
>    config_host_data.set('HAVE_COPY_FILE_RANGE',
>                         cc.has_function('copy_file_range'))
>
> Maybe some header is missing in "osdep.h" for GNU Hurd?
>
> > Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 35684f7e21..05426abb7d 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1999,7 +1999,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
> >       return handle_aiocb_write_zeroes(aiocb);
> >   }
> >
> > -#ifndef HAVE_COPY_FILE_RANGE
> > +#if !defined(HAVE_COPY_FILE_RANGE) && !defined(__GNU__)
> >   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)
> >   {
> > ---
> >   block/file-posix.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 35684f7e21..05426abb7d 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1999,7 +1999,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
> >       return handle_aiocb_write_zeroes(aiocb);
> >   }
> >
> > -#ifndef HAVE_COPY_FILE_RANGE
> > +#if !defined(HAVE_COPY_FILE_RANGE) && !defined(__GNU__)
> >   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)
> >   {
> > --
> > 2.43.0
> >
>

Re: [PATCH 2/4] Avoid multiple definitions of copy_file_range

[Samuel Thibault]

Manolo de Medici, le mer. 17 janv. 2024 15:09:39 +0100, a ecrit:
> Hello Philippe,
> thank you for the feedback, I've checked that. The problem is that the
> Hurd fails that test due to the following:
> 
>         #if defined __stub_copy_file_range || defined __stub___copy_file_range
>         fail fail fail this function is not going to work
>         #endifefines the stub __copy_file_ran
> 
> rightfully so I'd say, because copy_file_range is just a stub on the Hurd.

Yes.

> As such, we really need to exclude the code that defines the stub in
> qemu on the Hurd.

But how do things work without the qemu stub?

Or put another way: what problem exactly the presence of the qemu stub
makes?

Samuel

> On Wed, Jan 17, 2024 at 2:56 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> >
> > Hi Manolo,
> >
> > On 17/1/24 13:31, Manolo de Medici wrote:
> > > It's already defined as a stub on the GNU Hurd.
> >
> > Meson checks for this function and defines
> > HAVE_COPY_FILE_RANGE if available, see in meson.build:
> >
> >    config_host_data.set('HAVE_COPY_FILE_RANGE',
> >                         cc.has_function('copy_file_range'))
> >
> > Maybe some header is missing in "osdep.h" for GNU Hurd?
> >
> > > Signed-off-by: Manolo de Medici <manolo.demedici@gmail.com>
> > >
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 35684f7e21..05426abb7d 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -1999,7 +1999,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
> > >       return handle_aiocb_write_zeroes(aiocb);
> > >   }
> > >
> > > -#ifndef HAVE_COPY_FILE_RANGE
> > > +#if !defined(HAVE_COPY_FILE_RANGE) && !defined(__GNU__)
> > >   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)
> > >   {
> > > ---
> > >   block/file-posix.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 35684f7e21..05426abb7d 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -1999,7 +1999,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
> > >       return handle_aiocb_write_zeroes(aiocb);
> > >   }
> > >
> > > -#ifndef HAVE_COPY_FILE_RANGE
> > > +#if !defined(HAVE_COPY_FILE_RANGE) && !defined(__GNU__)
> > >   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)
> > >   {
> > > --
> > > 2.43.0
> > >
> >

Re: [PATCH 2/4] Avoid multiple definitions of copy_file_range

[Manolo de Medici]

Hello Samuel,
the presence of the qemu stub causes the following compilation error:

FAILED: libblock.fa.p/block_file-posix.c.o
cc -m32 -Ilibblock.fa.p -I. -I../../.. -Iqapi -Itrace -Iui -Iui/shader
-Iblock -I/usr/include/glib-2.0 -I/usr/lib/i386-gnu/glib-2.0/include
-fdiagnostics-color=auto -Wall -Winvalid-pch -std=gnu11 -O0 -g
-fstack-protector-strong -Wundef -Wwrite-strings -Wmissing-prototypes
-Wstrict-prototypes -Wredundant-decls -Wold-style-declaration
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
-Wmissing-format-attribute -Wno-missing-include-dirs
-Wno-shift-negative-value -Wno-psabi -Wshadow=local -iquote . -iquote
/root/qemu -iquote /root/qemu/include -iquote
/root/qemu/host/include/i386 -iquote /root/qemu/host/include/generic
-iquote /root/qemu/tcg/i386 -pthread -D_GNU_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing
-fno-common -fwrapv -fPIE -MD -MQ libblock.fa.p/block_file-posix.c.o
-MF libblock.fa.p/block_file-posix.c.o.d -o
libblock.fa.p/block_file-posix.c.o -c ../../../block/file-posix.c
../../../block/file-posix.c: In function 'probe_logical_blocksize':
../../../block/file-posix.c:310:13: warning: implicit declaration of
function 'ioctl' [-Wimplicit-function-declaration]
  310 |         if (ioctl(fd, ioctl_list[i], &sector_size) >= 0) {
      |             ^~~~~
../../../block/file-posix.c:310:13: warning: nested extern declaration
of 'ioctl' [-Wnested-externs]
../../../block/file-posix.c: At top level:
../../../block/file-posix.c:2003:14: error: conflicting types for
'copy_file_range'; have 'off_t(int,  off_t *, int,  off_t *, size_t,
unsigned int)' {aka 'long long int(int,  long long int *, int,  long
long int *, unsigned int,  unsigned int)'}
 2003 | static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
      |              ^~~~~~~~~~~~~~~
In file included from /root/qemu/include/qemu/osdep.h:122,
                 from ../../../block/file-posix.c:25:
/usr/include/unistd.h:1142:9: note: previous declaration of
'copy_file_range' with type 'ssize_t(int,  __off64_t *, int,
__off64_t *, size_t,  unsigned int)' {aka 'int(int,  long long int *,
int,  long long int *, unsigned int,  unsigned int)'}
 1142 | ssize_t copy_file_range (int __infd, __off64_t *__pinoff,
      |         ^~~~~~~~~~~~~~~

the current patch fixes this compilation error.

The resulting binary is the same as it would have been with only the qemu patch,
so I'm assuming that it is fully functional. Although I cannot be 100%
sure about that.

Re: [PATCH 2/4] Avoid multiple definitions of copy_file_range

[Samuel Thibault]

Hello,

Manolo de Medici, le mer. 17 janv. 2024 15:47:09 +0100, a ecrit:
> ../../../block/file-posix.c:2003:14: error: conflicting types for
> 'copy_file_range'; have 'off_t(int,  off_t *, int,  off_t *, size_t,
> unsigned int)' {aka 'long long int(int,  long long int *, int,  long
> long int *, unsigned int,  unsigned int)'}
>  2003 | static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
>       |              ^~~~~~~~~~~~~~~
> In file included from /root/qemu/include/qemu/osdep.h:122,
>                  from ../../../block/file-posix.c:25:
> /usr/include/unistd.h:1142:9: note: previous declaration of
> 'copy_file_range' with type 'ssize_t(int,  __off64_t *, int,
> __off64_t *, size_t,  unsigned int)' {aka 'int(int,  long long int *,
> int,  long long int *, unsigned int,  unsigned int)'}
>  1142 | ssize_t copy_file_range (int __infd, __off64_t *__pinoff,
>       |         ^~~~~~~~~~~~~~~
> 
> the current patch fixes this compilation error.

Yes, but by ignoring the difference :)

The prototype of copy_file_range in glibc really does say that it
returns an ssize_t, not an off_t, so that should be fixed so.

Samuel

Re: [PATCH 2/4] Avoid multiple definitions of copy_file_range

[Manolo de Medici]

Understood, but I cannot judge if it is a bug in qemu or it fixes
another host os,
since qemu doesn't target only glibc.

In order to avoid breaking other hosts, I consider it more cautious to
ignore the difference.
In the long term the Hurd is going to implement copy_file_range

Re: [PATCH 2/4] Avoid multiple definitions of copy_file_range

[Samuel Thibault]

Manolo de Medici, le mer. 17 janv. 2024 16:08:34 +0100, a ecrit:
> Understood, but I cannot judge if it is a bug in qemu or it fixes
> another host os,
> since qemu doesn't target only glibc.

Yes, but freebsd too uses ssize_t:

https://man.freebsd.org/cgi/man.cgi?copy_file_range(2)

glib mentions that it only exists on linux and freebsd.

https://www.gnu.org/software/gnulib/manual/html_node/copy_005ffile_005frange.html

> In order to avoid breaking other hosts, I consider it more cautious to
> ignore the difference.

Ignoring a bug is not a good thing on the long run :)

When there is something suspicious, it's useful to fix it.

> In the long term the Hurd is going to implement copy_file_range

Yes and by just fixing the prototype, we'll keep qemu able to
automatically use it when it becomes available.

Really, please, no tinkering, rather fix bugs.

Samuel

Re: [PATCH 2/4] Avoid multiple definitions of copy_file_range

[Manolo de Medici]

Ok Samuel,
in the v2 of this patch series I will fix this bug in qemu.