[libvirt] [PATCH 2/2] storage: Rename btrfsCloneFile to support other filesystems.

[Michal Privoznik]

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 at gmail.com>
>>> ---
>>>  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