On Tue, 2020-12-01 at 14:35 -0800, [email protected] wrote:
> Refactor the too large IOCTL function to call helper functions.
[]
> diff --git a/drivers/misc/xlink-core/xlink-ioctl.c
> b/drivers/misc/xlink-core/xlink-ioctl.c
[]
> +int ioctl_write_data(unsigned long arg)
> +{
> + struct xlink_handle devh = {0};
> + struct xlinkwritedata wr = {0};
> + int rc = 0;
> +
> + if (copy_from_user(&wr, (void __user *)arg,
> + sizeof(struct xlinkwritedata)))
> + return -EFAULT;
> + if (copy_from_user(&devh, (void __user *)wr.handle,
> + sizeof(struct xlink_handle)))
> + return -EFAULT;
> + if (wr.size <= XLINK_MAX_DATA_SIZE) {
> + rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage,
> + wr.size);
> + if (copy_to_user((void __user *)wr.return_code, (void *)&rc,
> + sizeof(rc)))
> + return -EFAULT;
> + } else {
> + return -EFAULT;
> + }
Please reverse the test to reduce indentation
if (wr.size > XLINK_MAX_DATA_SIZE)
return -EFAULT;
rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, wr.size);
if (copy_to_user((void __user *)wr.return_code, (void *)&rc,
sizeof(rc)))
return -EFAULT;
return rc;
The last 3 lines here are repeated multiple times in many functions.
It might be sensible to add something like:
int copy_result_to_user(u32 *where, int rc)
{
if (copy_to_user((void __user *)where, &rc, sizeof(rc)))
return -EFAULT;
return rc;
}
so this could be written
rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, wr.size);
return copy_result_to_user(wr.return_code, rc);
IMO: return_code isn't a great name for a pointer as it rather
indicates a value not an address and there's an awful lot of
casting to __user in all this code that perhaps should be marked
in the struct definitions rather than inside the function uses.
> +}
> +
> +int ioctl_write_control_data(unsigned long arg)
> +{
> + struct xlink_handle devh = {0};
All of these initializations with {0} should use {} instead as
the first element of whatever struct is not guaranteed to be
assignable as an int and gcc/clang guarantee 0 initialization
> + struct xlinkwritedata wr = {0};
> + u8 volbuf[XLINK_MAX_BUF_SIZE];
> + int rc = 0;
> +
> + if (copy_from_user(&wr, (void __user *)arg,
> + sizeof(struct xlinkwritedata)))
> + return -EFAULT;
> + if (copy_from_user(&devh, (void __user *)wr.handle,
> + sizeof(struct xlink_handle)))
> + return -EFAULT;
> + if (wr.size <= XLINK_MAX_CONTROL_DATA_SIZE) {
> + if (copy_from_user(volbuf, (void __user *)wr.pmessage,
> + wr.size))
> + return -EFAULT;
> + rc = xlink_write_control_data(&devh, wr.chan, volbuf,
> + wr.size);
> + if (copy_to_user((void __user *)wr.return_code,
> + (void *)&rc, sizeof(rc)))
> + return -EFAULT;
> + } else {
> + return -EFAULT;
Same test reversal and deindentation please.
> + }
> + return rc;
> +}
> +
