Re: [PATCH 1/6] scsi_cmnd: reinstate support for cmd_len > 32

[Bart Van Assche <bvanassche@acm.org>]

On 4/7/22 20:56, Douglas Gilbert wrote:
>   static int scsi_eh_tur(struct scsi_cmnd *scmd)
>   {
> -	static unsigned char tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0};
> +	static const u8 tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0};
>   	int retry_cnt = 1;
>   	enum scsi_disposition rtn;
>   
>   retry_tur:
> -	rtn = scsi_send_eh_cmnd(scmd, tur_command, 6,
> -				scmd->device->eh_timeout, 0);
> +	rtn = scsi_send_eh_cmnd(scmd, (u8 *)tur_command, 6, scmd->device->eh_timeout, 0);

Does the cast in the above function call cast away constness? There must 
be a better solution than casting away constness.

> -bool scsi_cmd_allowed(unsigned char *cmd, fmode_t mode)
> +bool scsi_cmd_allowed(/* const */ unsigned char *cmd, fmode_t mode)
>   {

Why has 'const' been commented out?

> @@ -1460,6 +1462,7 @@ static void scsi_complete(struct request *rq)
>   static int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
>   {
>   	struct Scsi_Host *host = cmd->device->host;
> +	u8 *cdb = (u8 *)scsi_cmnd_get_cdb(cmd);
>   	int rtn = 0;

Casting away constness is ugly. Consider providing two versions of 
scsi_cmnd_get_cdb(): one version that accepts a const pointer and 
returns a const pointer and another version that accepts a non-const 
pointer and returns a non-const pointer. Maybe _Generic() or 
__same_type() can be used to combine both versions into a single macro?

> +/* This value is used to size a C array, see below if cdb length > 32 */
> +#define SCSI_MAX_COMPILE_TIME_CDB_LEN 32

Since CDBs longer than 16 bytes are rare, how about using 16 as the 
maximum compile-time CDB size?

Thanks,

Bart.