[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Sunil Mushran]

Tristan Ye wrote:
> We implement the request as simple as possible to avoid
> a possible extending if disk format get changed.
>
> Completenss of a request include the info required and a flag(FL_FILLED)
> filled being returned to the end-user.
>
> Note: the requests in a series from userspace should be NULL-terminated.

Pass in the buffer length. Expecting null-termination is asking for trouble.

>
> Signed-off-by: Tristan Ye <tristan.ye at oracle.com>
> ---
>  fs/ocfs2/ioctl.c |  184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 184 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
> index 31fbb06..0ac5218 100644
> --- a/fs/ocfs2/ioctl.c
> +++ b/fs/ocfs2/ioctl.c
> @@ -108,6 +108,188 @@ bail:
>  	return status;
>  }
>  
> +static int ocfs2_get_request_info(struct inode *inode,
> +				  unsigned long arg)
> +{
> +
> +	int ret = 0, i = 0, num_reqs = 0;
> +
> +	struct ocfs2_info_request req, *preq = NULL;
> +	struct ocfs2_info_request **reqs;
> +	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> +
> +	typedef struct ocfs2_info_request OIR, *POIR;
> +	typedef struct ocfs2_info_request_clustersize OIRC, *POIRC;
> +	typedef struct ocfs2_info_request_blocksize OIRB, *POIRB;
> +	typedef struct ocfs2_info_request_slotnum OIRS, *POIRS;
> +	typedef struct ocfs2_info_request_label OIRL, *POIRL;
> +	typedef struct ocfs2_info_request_uuid OIRU, *POIRU;
> +	typedef struct ocfs2_info_request_fs_features OIRF, *POIRF;
> +

Three no-nos.
1. Do not typedef structures.
2. Use all-caps only for #defines.
3. Do not pre-pend type to the variable name.

For more refer to the kernel coding style.
http://lxr.linux.no/source/Documentation/CodingStyle

> +	/*
> +	 * The requests series from userspace need to be NULL-terminated.
> +	 */
> +	do {
> +		preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
> +		if (!preq)
> +			break;
> +		i++;
> +
> +	} while (preq);
> +
> +	num_reqs = i;

If we pass in the length of the buffer, then num_reqs = len / 
sizeof(struct ...);

> +
> +	reqs = kmalloc(sizeof(POIR) * num_reqs, GFP_KERNEL);
> +	if (!reqs) {
> +		ret = -ENOMEM;
> +		goto bail;
> +	}

We can then copy_from_user the buffer in one go. The remaining
code have to change accordingly.

> +
> +	ret = ocfs2_inode_lock(inode, NULL, 0);
> +	if (ret < 0) {
> +		mlog_errno(ret);
> +		goto bail;
> +	}

The superblock lock is special. See ocfs2_super_lock().

You'll need to traverse to the osb to use super_lock.
struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);

Also, make use of OCFS2_INFO_FL_NON_COHERENT.

> +
> +	for (i = 0; i < num_reqs; i++) {
> +
> +		reqs[i] = NULL;
> +
> +		preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
> +
> +		if (copy_from_user(&req, preq, sizeof(req))) {
> +			ret = -EFAULT;
> +			goto bail;
> +		}
> +
> +		switch (req.ir_code) {
> +		case OCFS2_INFO_CLUSTERSIZE:
> +			reqs[i] = kmalloc(sizeof(OIRC), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRC))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			((POIRC)(reqs[i]))->ir_clustersize = osb->s_clustersize;
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRC))) {
> +				ret = EFAULT;

-EFAULT.

> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_BLOCKSIZE:
> +			reqs[i] = kmalloc(sizeof(OIRB), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRB))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			((POIRB)(reqs[i]))->ir_blocksize =
> +					inode->i_sb->s_blocksize;
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRB))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_SLOTNUM:
> +			reqs[i] = kmalloc(sizeof(OIRS), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRS))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			((POIRS)(reqs[i]))->ir_slotnum = osb->max_slots;
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRS))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_LABEL:
> +			reqs[i] = kmalloc(sizeof(OIRL), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRL))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			memmove(((POIRL)(reqs[i]))->ir_label, osb->vol_label,
> +				OCFS2_MAX_VOL_LABEL_LEN);

Why memmove() when memcpy() will do?

> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRL))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_UUID:
> +			reqs[i] = kmalloc(sizeof(OIRU), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRU))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			memmove(((POIRU)(reqs[i]))->ir_uuid, osb->uuid,
> +				OCFS2_VOL_UUID_LEN);
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRU))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_FS_FEATURES:
> +			reqs[i] = kmalloc(sizeof(OIRF), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRF))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			((POIRF)(reqs[i]))->ir_compat_features =
> +						osb->s_feature_compat;
> +			((POIRF)(reqs[i]))->ir_incompat_features =
> +						osb->s_feature_incompat;
> +			((POIRF)(reqs[i]))->ir_ro_compat_features =
> +						osb->s_feature_ro_compat;
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRF))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		default:
> +			break;
> +		}
> +
> +	}
> +bail:
> +	for (i = 0; i < num_reqs; i++)
> +		kfree(reqs[i]);
> +
> +	kfree(reqs);
> +
> +	ocfs2_inode_unlock(inode, 0);
> +
> +	mlog_exit(ret);
> +	return ret;
> +}
> +
>  long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>  	struct inode *inode = filp->f_path.dentry->d_inode;
> @@ -173,6 +355,8 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		preserve = (args.preserve != 0);
>  
>  		return ocfs2_reflink_ioctl(inode, old_path, new_path, preserve);
> +	case OCFS2_IOC_INFO:
> +		return ocfs2_get_request_info(inode, arg);
>  	default:
>  		return -ENOTTY;
>  	}