| Message ID | 20240723-ioctl_ficlone-v2-1-33075bbc117f@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add ioctl_ficlone testing suite | expand |
Hi! > #include <limits.h> I've added #include <stdint.h> here to make sure that the *int*_t are defined and pushed, thanks. > #include "lapi/abisize.h" > > +#ifndef HAVE_FILE_CLONE_RANGE > +struct file_clone_range { > + int64_t src_fd; > + uint64_t src_offset; > + uint64_t src_length; > + uint64_t dest_offset; > +}; > +#endif
Hi! > diff --git a/configure.ac b/configure.ac > index 1f8796c87..4d466f052 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -177,6 +177,7 @@ AC_CHECK_TYPES([struct acct_v3],,,[#include <sys/acct.h>]) > AC_CHECK_TYPES([struct af_alg_iv, struct sockaddr_alg],,,[# include <linux/if_alg.h>]) > AC_CHECK_TYPES([struct fanotify_event_info_fid, struct fanotify_event_info_error, > struct fanotify_event_info_header, struct fanotify_event_info_pidfd],,,[#include <sys/fanotify.h>]) > +AC_CHECK_TYPES([struct file_clone_range],,,[#include <linux/fs.h>]) > AC_CHECK_TYPES([struct file_dedupe_range],,,[#include <linux/fs.h>]) > > AC_CHECK_TYPES([struct file_handle],,,[ > diff --git a/include/lapi/fs.h b/include/lapi/fs.h > index 635979b02..0e8d646d8 100644 > --- a/include/lapi/fs.h > +++ b/include/lapi/fs.h > @@ -20,6 +20,15 @@ > #include <limits.h> > #include "lapi/abisize.h" > > +#ifndef HAVE_FILE_CLONE_RANGE > +struct file_clone_range { > + int64_t src_fd; > + uint64_t src_offset; > + uint64_t src_length; > + uint64_t dest_offset; > +}; > +#endif Sigh, this is still horribly broken even when I fixed the macro to HAVE_STRUCT_FILE_CLONE_RANGE that is because: commit b857f8723f30a4b9554bf6b0ff8fa52fd07e8b60 Author: Li Wang <liwang@redhat.com> Date: Fri Aug 5 14:34:01 2022 +0800 lapi/fsmount: resolve conflict in different header files However the CI seems to work fine if I remove the HAVE_MOUNT_SETATTR check with: #define LAPI_FS_H__ #include "config.h" -#ifndef HAVE_MOUNT_SETATTR -# ifdef HAVE_LINUX_FS_H -# include <linux/fs.h> -# endif + +#ifdef HAVE_LINUX_FS_H +# include <linux/fs.h> #endif #include <sys/user.h> @Li do we still need that ifdef or can we get rid of it?
On Thu, Jul 25, 2024 at 12:29 AM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > diff --git a/configure.ac b/configure.ac > > index 1f8796c87..4d466f052 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -177,6 +177,7 @@ AC_CHECK_TYPES([struct acct_v3],,,[#include > <sys/acct.h>]) > > AC_CHECK_TYPES([struct af_alg_iv, struct sockaddr_alg],,,[# include > <linux/if_alg.h>]) > > AC_CHECK_TYPES([struct fanotify_event_info_fid, struct > fanotify_event_info_error, > > struct fanotify_event_info_header, struct > fanotify_event_info_pidfd],,,[#include <sys/fanotify.h>]) > > +AC_CHECK_TYPES([struct file_clone_range],,,[#include <linux/fs.h>]) > > AC_CHECK_TYPES([struct file_dedupe_range],,,[#include <linux/fs.h>]) > > > > AC_CHECK_TYPES([struct file_handle],,,[ > > diff --git a/include/lapi/fs.h b/include/lapi/fs.h > > index 635979b02..0e8d646d8 100644 > > --- a/include/lapi/fs.h > > +++ b/include/lapi/fs.h > > @@ -20,6 +20,15 @@ > > #include <limits.h> > > #include "lapi/abisize.h" > > > > +#ifndef HAVE_FILE_CLONE_RANGE > > +struct file_clone_range { > > + int64_t src_fd; > > + uint64_t src_offset; > > + uint64_t src_length; > > + uint64_t dest_offset; > > +}; > > +#endif > > Sigh, this is still horribly broken even when I fixed the macro to > HAVE_STRUCT_FILE_CLONE_RANGE that is because: > > commit b857f8723f30a4b9554bf6b0ff8fa52fd07e8b60 > Author: Li Wang <liwang@redhat.com> > Date: Fri Aug 5 14:34:01 2022 +0800 > > lapi/fsmount: resolve conflict in different header files > > > However the CI seems to work fine if I remove the HAVE_MOUNT_SETATTR > check with: > > #define LAPI_FS_H__ > > #include "config.h" > -#ifndef HAVE_MOUNT_SETATTR > -# ifdef HAVE_LINUX_FS_H > -# include <linux/fs.h> > -# endif > + > +#ifdef HAVE_LINUX_FS_H > +# include <linux/fs.h> > #endif > > #include <sys/user.h> > > > @Li do we still need that ifdef or can we get rid of it? > Theoretically, we can remove it because that problem is addressed in Glibc-2.37 by: https://github.com/kraj/glibc/commit/774058d72942249f71d74e7f2b639f77184160a6 It is essentially a glibc bug we don't need to fix that in LTP. We ever discussed in https://lists.linux.it/pipermail/ltp/2023-March/033138.html However, if we want LTP could be built with all the middle glibc versions (2.22 < glibc < 2.36) this might be thinking over. Because we announce support the minimal glibc-version is 2.22.
Hi! > Theoretically, we can remove it because that problem is addressed in > Glibc-2.37 by: > > > https://github.com/kraj/glibc/commit/774058d72942249f71d74e7f2b639f77184160a6 > > It is essentially a glibc bug we don't need to fix that in LTP. > > We ever discussed in > https://lists.linux.it/pipermail/ltp/2023-March/033138.html > > However, if we want LTP could be built with all the middle glibc versions > (2.22 < glibc < 2.36) > this might be thinking over. Because we announce support the minimal > glibc-version is 2.22. That does sound like we will need the workaround for quite some time. Or maybe give up on including linux/fs.h and stick to the libc headers.
Hi all, ... > > #include "config.h" > > -#ifndef HAVE_MOUNT_SETATTR > > -# ifdef HAVE_LINUX_FS_H > > -# include <linux/fs.h> > > -# endif > > + > > +#ifdef HAVE_LINUX_FS_H > > +# include <linux/fs.h> > > #endif > > #include <sys/user.h> > > @Li do we still need that ifdef or can we get rid of it? > Theoretically, we can remove it because that problem is addressed in > Glibc-2.37 by: > https://github.com/kraj/glibc/commit/774058d72942249f71d74e7f2b639f77184160a6 > It is essentially a glibc bug we don't need to fix that in LTP. > We ever discussed in > https://lists.linux.it/pipermail/ltp/2023-March/033138.html > However, if we want LTP could be built with all the middle glibc versions > (2.22 < glibc < 2.36) > this might be thinking over. Because we announce support the minimal > glibc-version is 2.22. Hm, it makes sense to keep it. But nobody will remember once we raise the support. Also, removing HAVE_LINUX_FS_H [1] works in the CI [2], including distros with glibc 2.36 (minimal build [3] or all cross-compile builds, e.g. [4]). I wonder how realistic is that somebody is still affected by this issue. Kind regards, Petr [1] https://github.com/linux-test-project/ltp/commit/4bc155448559bc2ff75381c0e04662d585677bc4 [2] https://github.com/pevik/ltp/actions/runs/10093238273 [3] https://github.com/pevik/ltp/actions/runs/10093238273/job/27908521308 [4] https://github.com/pevik/ltp/actions/runs/10093238273/job/27908520925
Hi! > > However, if we want LTP could be built with all the middle glibc versions > > (2.22 < glibc < 2.36) > > this might be thinking over. Because we announce support the minimal > > glibc-version is 2.22. > > Hm, it makes sense to keep it. But nobody will remember once we raise the > support. Maybe we should at least add a comment glibc-2.22 workaround or something that could be found with grep. > Also, removing HAVE_LINUX_FS_H [1] works in the CI [2], including distros with > glibc 2.36 (minimal build [3] or all cross-compile builds, e.g. [4]). > > I wonder how realistic is that somebody is still affected by this issue. That's a good question but I'm afraid the only way to find out is to remove the workaround and wait for people complain that the next LTP release is broken...
On Thu, Jul 25, 2024 at 11:17 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > > However, if we want LTP could be built with all the middle glibc > versions > > > (2.22 < glibc < 2.36) > > > this might be thinking over. Because we announce support the minimal > > > glibc-version is 2.22. > > > > Hm, it makes sense to keep it. But nobody will remember once we raise the > > support. > > Maybe we should at least add a comment glibc-2.22 workaround or > something that could be found with grep. > Sounds good. > > Also, removing HAVE_LINUX_FS_H [1] works in the CI [2], including > distros with > > glibc 2.36 (minimal build [3] or all cross-compile builds, e.g. [4]). > > > > I wonder how realistic is that somebody is still affected by this issue. > > That's a good question but I'm afraid the only way to find out is to > remove the workaround and wait for people complain that the next LTP > release is broken... > I remember the broken was found by our CI automation tests on Fedora-rawhide at that moment. That is a tentative version and fixed imitatively by my patch, so yes, we can remove that to see if it impacts other users.
diff --git a/configure.ac b/configure.ac index 1f8796c87..4d466f052 100644 --- a/configure.ac +++ b/configure.ac @@ -177,6 +177,7 @@ AC_CHECK_TYPES([struct acct_v3],,,[#include <sys/acct.h>]) AC_CHECK_TYPES([struct af_alg_iv, struct sockaddr_alg],,,[# include <linux/if_alg.h>]) AC_CHECK_TYPES([struct fanotify_event_info_fid, struct fanotify_event_info_error, struct fanotify_event_info_header, struct fanotify_event_info_pidfd],,,[#include <sys/fanotify.h>]) +AC_CHECK_TYPES([struct file_clone_range],,,[#include <linux/fs.h>]) AC_CHECK_TYPES([struct file_dedupe_range],,,[#include <linux/fs.h>]) AC_CHECK_TYPES([struct file_handle],,,[ diff --git a/include/lapi/fs.h b/include/lapi/fs.h index 635979b02..0e8d646d8 100644 --- a/include/lapi/fs.h +++ b/include/lapi/fs.h @@ -20,6 +20,15 @@ #include <limits.h> #include "lapi/abisize.h" +#ifndef HAVE_FILE_CLONE_RANGE +struct file_clone_range { + int64_t src_fd; + uint64_t src_offset; + uint64_t src_length; + uint64_t dest_offset; +}; +#endif + #ifndef FS_IOC_GETFLAGS # define FS_IOC_GETFLAGS _IOR('f', 1, long) #endif @@ -48,6 +57,14 @@ # define FS_VERITY_FL 0x00100000 /* Verity protected inode */ #endif +#ifndef FICLONE +# define FICLONE _IOW(0x94, 9, int) +#endif + +#ifndef FICLONERANGE +# define FICLONERANGE _IOW(0x94, 13, struct file_clone_range) +#endif + /* * Helper function to get MAX_LFS_FILESIZE. * Missing PAGE_SHIFT on some libc prevents defining MAX_LFS_FILESIZE.