[PATCH] btrfs-progs: fix btrfs quota rescan failed on PPC64 arch

[PATCH] btrfs-progs: fix btrfs quota rescan failed on PPC64 arch

[xuw2015]

From: George Wang <xuw2015@gmail.com>

PPC64 arch use such following IOC values "
\#define _IOC_NONE       1U
\#define _IOC_READ       2U
\#define _IOC_WRITE      4U
" comparing to the default IOC values "
\#define _IOC_NONE       0U
\#define _IOC_READ       2U
\#define _IOC_WRITE      1U"

This means the value "_IOW*" will be negative when we store it in the int
variables. Such as the "BTRFS_IOC_QGROUP_CREATE", it will be "0x4010942e" on
X86_64, but "0x8010942e" on PPC64.
Notice that the IOC values are the "unsigned long" type, so we use the
"unsigned long" to store it, and this can insure the comparison between the
variable and BTRFS_IOC_* valid.

Signed-off-by: George Wang <xuw2015@gmail.com>
---
 cmds-quota.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds-quota.c b/cmds-quota.c
index 89cc89c..f6a1cfa 100644
--- a/cmds-quota.c
+++ b/cmds-quota.c
@@ -109,7 +109,7 @@ static int cmd_quota_rescan(int argc, char **argv)
 	int e;
 	char *path = NULL;
 	struct btrfs_ioctl_quota_rescan_args args;
-	int ioctlnum = BTRFS_IOC_QUOTA_RESCAN;
+	unsigned long ioctlnum = BTRFS_IOC_QUOTA_RESCAN;
 	DIR *dirstream = NULL;
 	int wait_for_completion = 0;
 
-- 
1.9.3

Re: [PATCH] btrfs-progs: fix btrfs quota rescan failed on PPC64 arch

[Eric Sandeen]

On 4/20/15 12:33 AM, xuw2015@gmail.com wrote:
> From: George Wang <xuw2015@gmail.com>
> 
> PPC64 arch use such following IOC values "
> \#define _IOC_NONE       1U
> \#define _IOC_READ       2U
> \#define _IOC_WRITE      4U
> " comparing to the default IOC values "
> \#define _IOC_NONE       0U
> \#define _IOC_READ       2U
> \#define _IOC_WRITE      1U"
> 
> This means the value "_IOW*" will be negative when we store it in the int
> variables. Such as the "BTRFS_IOC_QGROUP_CREATE", it will be "0x4010942e" on
> X86_64, but "0x8010942e" on PPC64.
> Notice that the IOC values are the "unsigned long" type, so we use the
> "unsigned long" to store it, and this can insure the comparison between the
> variable and BTRFS_IOC_* valid.

Looks good - very strange that the manpage states that the interface takes
an int.  :(  

But - an "unsigned int" would be enough, right?  I don't think it needs
to be an unsigned long.  *shrug*

Thanks,
-Eric

> Signed-off-by: George Wang <xuw2015@gmail.com>
> ---
>  cmds-quota.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cmds-quota.c b/cmds-quota.c
> index 89cc89c..f6a1cfa 100644
> --- a/cmds-quota.c
> +++ b/cmds-quota.c
> @@ -109,7 +109,7 @@ static int cmd_quota_rescan(int argc, char **argv)
>  	int e;
>  	char *path = NULL;
>  	struct btrfs_ioctl_quota_rescan_args args;
> -	int ioctlnum = BTRFS_IOC_QUOTA_RESCAN;
> +	unsigned long ioctlnum = BTRFS_IOC_QUOTA_RESCAN;
>  	DIR *dirstream = NULL;
>  	int wait_for_completion = 0;
>  
> 

Re: [PATCH] btrfs-progs: fix btrfs quota rescan failed on PPC64 arch

[王旭]

Thanks for review.

On Mon, Apr 20, 2015 at 10:47 PM, Eric Sandeen <sandeen@redhat.com> wrote:
> On 4/20/15 12:33 AM, xuw2015@gmail.com wrote:
>> From: George Wang <xuw2015@gmail.com>
<snip>
>> This means the value "_IOW*" will be negative when we store it in the int
>> variables. Such as the "BTRFS_IOC_QGROUP_CREATE", it will be "0x4010942e" on
>> X86_64, but "0x8010942e" on PPC64.
>> Notice that the IOC values are the "unsigned long" type, so we use the
>> "unsigned long" to store it, and this can insure the comparison between the
>> variable and BTRFS_IOC_* valid.
>
> Looks good - very strange that the manpage states that the interface takes
> an int.  :(

I think maybe the manpage of ioctl is stale. But int can also work
except for comparison.
And fortunately the kernel interface is unsigned int, such as btrfs:

5217 long btrfs_ioctl(struct file *file, unsigned int
5218         cmd, unsigned long arg)

>
> But - an "unsigned int" would be enough, right?  I don't think it needs
> to be an unsigned long.  *shrug*

unsigned int will be OK. But the btrfs-progs is user space, so the
ioctl I think it should be
consistent with the glibc:
/usr/include/sys/ioctl.h:extern int ioctl (int __fd, unsigned long int
__request, ...) __THROW;
unsigned long int is same with unsigned long. So maybe the unsigned
long will be better?

Thanks,
George

Re: [PATCH] btrfs-progs: fix btrfs quota rescan failed on PPC64 arch

[David Sterba]

On Tue, Apr 21, 2015 at 10:26:23AM +0800, 王旭 wrote:
> > On 4/20/15 12:33 AM, xuw2015@gmail.com wrote:
> >> From: George Wang <xuw2015@gmail.com>
> <snip>
> >> This means the value "_IOW*" will be negative when we store it in the int
> >> variables. Such as the "BTRFS_IOC_QGROUP_CREATE", it will be "0x4010942e" on
> >> X86_64, but "0x8010942e" on PPC64.
> >> Notice that the IOC values are the "unsigned long" type, so we use the
> >> "unsigned long" to store it, and this can insure the comparison between the
> >> variable and BTRFS_IOC_* valid.
> >
> > Looks good - very strange that the manpage states that the interface takes
> > an int.  :(
> 
> I think maybe the manpage of ioctl is stale. But int can also work
> except for comparison.
> And fortunately the kernel interface is unsigned int, such as btrfs:
> 
> 5217 long btrfs_ioctl(struct file *file, unsigned int
> 5218         cmd, unsigned long arg)
> 
> >
> > But - an "unsigned int" would be enough, right?  I don't think it needs
> > to be an unsigned long.  *shrug*
> 
> unsigned int will be OK. But the btrfs-progs is user space, so the
> ioctl I think it should be
> consistent with the glibc:
> /usr/include/sys/ioctl.h:extern int ioctl (int __fd, unsigned long int
> __request, ...) __THROW;
> unsigned long int is same with unsigned long. So maybe the unsigned
> long will be better?

I think we should stick to the glibc interface as it's the closest one,
although kernel uses usigned int in the end. Thanks.

Re: [PATCH] btrfs-progs: fix btrfs quota rescan failed on PPC64 arch

[Chandan Rajendra]

On Monday 20 Apr 2015 13:33:16 xuw2015@gmail.com wrote:
> From: George Wang <xuw2015@gmail.com>
> 
> PPC64 arch use such following IOC values "
> \#define _IOC_NONE       1U
> \#define _IOC_READ       2U
> \#define _IOC_WRITE      4U
> " comparing to the default IOC values "
> \#define _IOC_NONE       0U
> \#define _IOC_READ       2U
> \#define _IOC_WRITE      1U"
> 
> This means the value "_IOW*" will be negative when we store it in the int
> variables. Such as the "BTRFS_IOC_QGROUP_CREATE", it will be "0x4010942e" on
> X86_64, but "0x8010942e" on PPC64.
> Notice that the IOC values are the "unsigned long" type, so we use the
> "unsigned long" to store it, and this can insure the comparison between the
> variable and BTRFS_IOC_* valid.
> 
> Signed-off-by: George Wang <xuw2015@gmail.com>

Tested-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

-- 
chandan

Re: [PATCH] btrfs-progs: fix btrfs quota rescan failed on PPC64 arch

[David Sterba]

On Thu, Apr 23, 2015 at 10:43:29PM +0530, Chandan Rajendra wrote:
> On Monday 20 Apr 2015 13:33:16 xuw2015@gmail.com wrote:
> > From: George Wang <xuw2015@gmail.com>
> > 
> > PPC64 arch use such following IOC values "
> > \#define _IOC_NONE       1U
> > \#define _IOC_READ       2U
> > \#define _IOC_WRITE      4U
> > " comparing to the default IOC values "
> > \#define _IOC_NONE       0U
> > \#define _IOC_READ       2U
> > \#define _IOC_WRITE      1U"
> > 
> > This means the value "_IOW*" will be negative when we store it in the int
> > variables. Such as the "BTRFS_IOC_QGROUP_CREATE", it will be "0x4010942e" on
> > X86_64, but "0x8010942e" on PPC64.
> > Notice that the IOC values are the "unsigned long" type, so we use the
> > "unsigned long" to store it, and this can insure the comparison between the
> > variable and BTRFS_IOC_* valid.
> > 
> > Signed-off-by: George Wang <xuw2015@gmail.com>
> 
> Tested-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

Thanks for testing, commit updated.