Re: [PATCH v2 01/15] scsi: Add struct for args to execution functions

[John Garry <john.g.garry@oracle.com>]

On 09/12/2022 18:47, Bart Van Assche wrote:
>>> around it then we have to do a WARN/BUG. We do the macro approach now
>>> so we can do the BUILD_BUG_ON.
>>
>> Maybe we have to switch to a WARN/BUG.
>>
>> It looks like some compilers don't like:
>>
>> const struct scsi_exec_args exec_args = {
>>     .sshdr = &sshdr,
>> };
>>
>> scsi_execute_args(.... exec_args);
>>
>> and will hit the:
>>
>> #define scsi_execute_args(sdev, cmd, opf, buffer, bufflen, timeout,     \
>>                            retries, 
>> args)                                \
>> ({                                                                      \
>>          BUILD_BUG_ON(args.sense 
>> &&                                      \
>>                       args.sense_len != 
>> SCSI_SENSE_BUFFERSIZE);          \
>>
>> because the args's sense and sense_len are not cleared yet.
> 
> My understanding is that the __scsi_execute() macro was introduced to 
> prevent that every single scsi_execute() caller would have to be 
> modified. I'm fine with removing the BUILD_BUG_ON(sense_len != 
> SCSI_SENSE_BUFFER_SIZE) check and replacing it with a WARN_ON_ONCE() 
> statement, e.g. inside __scsi_execute().

Another option is to have __scsi_execute() allocate the sense buf by 
kmemdup, and hold the sense pointer as unsigned char ** in struct 
scsi_exec_args; but then the caller needs to kfree the allocated sense 
buf, which I suppose is less than ideal. However there is only single 
driver which uses this AFAICS.

Thanks,
John