Messages in this thread | | | Subject | Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command | From | Joe Perches <> | Date | Wed, 09 Dec 2020 00:30:46 -0800 |
| |
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; > +} > +
| |