RE: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command

[Kelly, Seamus]

-----Original Message-----
From: Joe Perches <joe@xxxxxxxxxxx> 
Sent: Wednesday, December 9, 2020 8:31 AM
To: mgross@xxxxxxxxxxxxxxx; markgross@xxxxxxxxxx; arnd@xxxxxxxx; bp@xxxxxxx; damien.lemoal@xxxxxxx; dragan.cvetic@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; corbet@xxxxxxx; leonard.crestez@xxxxxxx; palmerdabbelt@xxxxxxxxxx; paul.walmsley@xxxxxxxxxx; peng.fan@xxxxxxx; robh+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx; Kelly, Seamus <seamus.kelly@xxxxxxxxx>
Subject: Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command

On Tue, 2020-12-01 at 14:35 -0800, mgross@xxxxxxxxxxxxxxx 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
[Kelly, Seamus]  Thank you!  will do.

	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);

[Kelly, Seamus] Thank you!  will do.

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.

[Kelly, Seamus] Thank you!  will do.
> +}
> +
> +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
[Kelly, Seamus] Thank you!  will do.

> +	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.
[Kelly, Seamus] Thank you!  will do.

> +	}
> +	return rc;
> +}
> +


--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.