| Message ID | 20181205091728.29903-5-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New ->fiemap infrastructure and ->bmap removal | expand |
On Wed, Dec 05, 2018 at 10:17:22AM +0100, Carlos Maiolino wrote: > Now we have the possibility of proper error return in bmap, use bmap() > function in ioctl_fibmap() instead of calling ->bmap method directly. > > V2: > - Use a local sector_t variable to asign the block number > instead of using direct casting. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/ioctl.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index d64f622cac8b..e0cc0dd5f9aa 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -53,19 +53,26 @@ EXPORT_SYMBOL(vfs_ioctl); > > static int ioctl_fibmap(struct file *filp, int __user *p) > { > + struct inode *inode = file_inode(filp); > + int error, usr_blk; > + sector_t block; > > if (!capable(CAP_SYS_RAWIO)) > return -EPERM; > + > + error = get_user(usr_blk, p); > + if (error) > + return error; Does get_user/put_user actually return an error? All the code I know just does: if (get_user())) return -EFAULT; and co. > + > + block = usr_blk; > + error = bmap(inode, &block); > + if (error) > + return error; > + usr_blk = block; Nitpick: maybe i'd name ur_block block and block sector, which seems to flow a little better.
Hi Christoph. Sorry for the delayed reply. > > + error = get_user(usr_blk, p); > > + if (error) > > + return error; > > Does get_user/put_user actually return an error? > > All the code I know just does: > > if (get_user())) > return -EFAULT; > > and co. According to the comment above it, it either returns zero on success, or -EFAULT on error. So, both approaches are correct, either mine, or the one you mentioned. It just seems more logical to me, to return whatever error code returned by the function (well, macro), instead of hardcoding -EFAULT. > > > + > > + block = usr_blk; > > + error = bmap(inode, &block); > > + if (error) > > + return error; > > + usr_blk = block; > > Nitpick: maybe i'd name ur_block block and block sector, which seems > to flow a little better. Sure, it's ok for me, I'll change that on a V3 >
diff --git a/fs/ioctl.c b/fs/ioctl.c index d64f622cac8b..e0cc0dd5f9aa 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -53,19 +53,26 @@ EXPORT_SYMBOL(vfs_ioctl); static int ioctl_fibmap(struct file *filp, int __user *p) { - struct address_space *mapping = filp->f_mapping; - int res, block; + struct inode *inode = file_inode(filp); + int error, usr_blk; + sector_t block; - /* do we support this mess? */ - if (!mapping->a_ops->bmap) - return -EINVAL; if (!capable(CAP_SYS_RAWIO)) return -EPERM; - res = get_user(block, p); - if (res) - return res; - res = mapping->a_ops->bmap(mapping, block); - return put_user(res, p); + + error = get_user(usr_blk, p); + if (error) + return error; + + block = usr_blk; + error = bmap(inode, &block); + if (error) + return error; + usr_blk = block; + + error = put_user(usr_blk, p); + + return error; } /**
Now we have the possibility of proper error return in bmap, use bmap() function in ioctl_fibmap() instead of calling ->bmap method directly. V2: - Use a local sector_t variable to asign the block number instead of using direct casting. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/ioctl.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)