On 07/09/2018 08:57 AM, Peter Krempa wrote: > On Fri, Jul 06, 2018 at 16:57:17 +0200, Michal Privoznik wrote: >> On 07/06/2018 03:43 PM, Julio Faracco wrote: >>> This commit renames and adds other macros to support aother filesystems >>> when a reflink is performed. After that, XFS filesystems (and others) >>> with reflink support will be able to clone. >>> >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1565004 >>> >>> Signed-off-by: Julio Faracco <jcfaracco@xxxxxxxxx> >>> --- >>> src/storage/storage_util.c | 20 ++++++++++++++------ >>> 1 file changed, 14 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c >>> index a701a75702..fd1239c6cb 100644 >>> --- a/src/storage/storage_util.c >>> +++ b/src/storage/storage_util.c >>> @@ -36,6 +36,9 @@ >>> # ifndef FS_NOCOW_FL >>> # define FS_NOCOW_FL 0x00800000 /* Do not cow file */ >>> # endif >>> +# ifdef FICLONE >>> +# define REFLINK_IOC_CLONE FICLONE >>> +# endif >>> #endif >>> >>> #if WITH_BLKID >>> @@ -48,6 +51,10 @@ >>> >>> #if HAVE_LINUX_BTRFS_H >>> # include <linux/btrfs.h> >>> +# define REFLINK_IOC_CLONE BTRFS_IOC_CLONE >>> +#elif HAVE_XFS_XFS_H >>> +# include <xfs/xfs.h> >>> +# define REFLINK_IOC_CLONE XFS_IOC_CLONE >>> #endif >>> >> >> Problem is, REFLING_IOC_CLONE is defined already at this point (by hunk >> above) so this redefines the macro. >> >> Fixed by squashing this in: >> >> diff --git i/src/storage/storage_util.c w/src/storage/storage_util.c >> index fd1239c6cb..da99043e0a 100644 >> --- i/src/storage/storage_util.c >> +++ w/src/storage/storage_util.c >> @@ -36,9 +36,6 @@ >> # ifndef FS_NOCOW_FL >> # define FS_NOCOW_FL 0x00800000 /* Do not cow file */ >> # endif >> -# ifdef FICLONE >> -# define REFLINK_IOC_CLONE FICLONE >> -# endif >> #endif >> >> #if WITH_BLKID >> @@ -55,6 +52,8 @@ >> #elif HAVE_XFS_XFS_H >> # include <xfs/xfs.h> >> # define REFLINK_IOC_CLONE XFS_IOC_CLONE >> +#elif defined(FICLONE) >> +# define REFLINK_IOC_CLONE FICLONE > > While it fortunately does not matter functionally as: > > /usr/include/linux/fs.h:#define FICLONE _IOW(0x94, 9, int) > > /usr/include/xfs/xfs_fs.h:#define XFS_IOC_CLONE _IOW (0x94, 9, int) > > /usr/include/linux/btrfs.h:#define BTRFS_IOCTL_MAGIC 0x94 > /usr/include/linux/btrfs.h:#define BTRFS_IOC_CLONE _IOW(BTRFS_IOCTL_MAGIC, 9, int) Huh. So why are filesystems providing their own symbol? Just to confuse users? > > I think the logic should be the other way around and use the > most-generic definition first. This is just plain confusing for readers > e.g. if you have both headers for XFS and BTRFS. For linux users it does > not matter if any of the others are defined as the FICLONE should always > be defined. Okay. I'll post a patch for that. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
