lkml.org 
[lkml]   [2020]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command
From
Date
On Tue, 2020-12-01 at 14:35 -0800, mgross@linux.intel.com 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;
> +}
> +


\
 
 \ /
  Last update: 2020-12-09 09:33    [W:1.025 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site