btrfs-progs: fix btrfs quota rescan failed on PPC64 arch
diff mbox

Message ID 1429507996-28224-1-git-send-email-xuw2015@gmail.com
State Accepted
Headers show

Commit Message

xuw2015@gmail.com April 20, 2015, 5:33 a.m. UTC
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(-)

Comments

Eric Sandeen April 20, 2015, 2:47 p.m. UTC | #1
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;
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
xuw2015@gmail.com April 21, 2015, 2:26 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba April 22, 2015, 4:04 p.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chandan Rajendra April 23, 2015, 5:13 p.m. UTC | #4
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>
David Sterba April 24, 2015, 1:42 p.m. UTC | #5
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;