Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support 32 bit FS_IOC32_{GET|SET}FLAGS compat ioctls #4477

Closed
wants to merge 1 commit into from
Closed

Add support 32 bit FS_IOC32_{GET|SET}FLAGS compat ioctls #4477

wants to merge 1 commit into from

Conversation

ColinIanKing
Copy link
Contributor

We need 32 bit userspace FS_IOC32_GETFLAGS and FS_IOC32_SETFLAGS
compat ioctls for systems such as powerpc64. We use the normal
compat ioctl idiom as used by a variety of file systems to provide
this support.

Signed-off-by: Colin Ian King [email protected]

We need 32 bit userspace FS_IOC32_GETFLAGS and FS_IOC32_SETFLAGS
compat ioctls for systems such as powerpc64.  We use the normal
compat ioctl idiom as used by a variety of file systems to provide
this support.

Signed-off-by: Colin Ian King <[email protected]>
cmd = FS_IOC_SETFLAGS;
break;
default:
return (-ENOTTY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the other Linux filesystems return -ENOIOCTLCMD in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autofs4, btrfs, ceph, cifs, hfsplus, jffs2, jffs, xfs return -ENOTTY, ncpfs returns -EINVAL, and ubifs returns -ENOTTY and also -ENOIOCTLCMD. The rest return -ENOIOCTLCMD. So, I'm not sure what the correct error return is. According to ioctl(2) manual, -ENOTTY seems like the documented return for a command that is not recognised for the specific object being operated on. So shall we leave it as -ENOTTY for now and not change that functionality in this particular commit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I was just basing my comment off what ext2/3/4 does. I'm glad you took a bigger sample. Yes, I'm OK leaving this as -ENOTTY. It can always be changed if needed in a latter commit if needed.

@behlendorf
Copy link
Contributor

There's going to be an identical problem to this in zvol_compat_ioctl() where ioctl's for volumes are handled (BLKFLSBUF, BLKZNAME). Let's tackle both of this cases in the same commit since they share a root cause.

@ColinIanKing
Copy link
Contributor Author

How do you want to play this? I'm running out of time on this today, it's rather late now UK time. I'm happy if you drop this patch and fix it appropriately for the zvol_compat_ioctl case and the more standard error return. I can then give it a spin first thing Friday morning. Or I can sort it out tomorrow. I'm easy either way.

@behlendorf
Copy link
Contributor

Sure, if you like I can open a new PR which makes the corresponding zvol_compat_ioctl() change. I don't have a test setup handy to verify it resolves the compat issue so you'll need to verify that Friday morning.

@behlendorf
Copy link
Contributor

And since your original patch then doesn't require any changes we could just leave the zvol_compat_ioctl() fixes for another patch. Although we should sort it out now while the issue is well understand and you're setup to verify it.

@ColinIanKing
Copy link
Contributor Author

Lets go for that zvol_compat_ioctl() as another patch which I can test sooner than later 'cos I've got access to some kit for another few more days. I have a window of a few more days and then we go for final freeze on fixes for 16.04.

@behlendorf
Copy link
Contributor

@ColinIanKing sounds good. I'll open a new PR with the zvol_compat_ioctl() fix in the next few hours so you should have it in the morning. If you have a list of other issues like this you've hit by all means let me know, I may be able to point you to fixes already in master.

@behlendorf behlendorf closed this in f7b939b Apr 1, 2016
@behlendorf
Copy link
Contributor

@ColinIanKing I took a closer look at zvol_compat_ioctl() and it doesn't look like there will be an issue. The generic Linux blkdev ioctl handler already deals with most of the standard ioctls(). The two cases that remain are:

  • BLKZNAME which is ZFS specific and takes a fixed length character array so there's no need for a compat version. And,
  • BLKFLSBUF which is defined by the kernel and implemented by ZFS, but it's just a command without arguments so again there is no issue.

I've merged your proposed patch to master, and we'll cherry-pick it back on to the release branch.

f7b939b Add support 32 bit FS_IOC32_{GET|SET}FLAGS compat ioctls

nedbass pushed a commit to nedbass/zfs that referenced this pull request May 6, 2016
We need 32 bit userspace FS_IOC32_GETFLAGS and FS_IOC32_SETFLAGS
compat ioctls for systems such as powerpc64.  We use the normal
compat ioctl idiom as used by a variety of file systems to provide
this support.

Signed-off-by: Colin Ian King <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#4477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants