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

[Mike Christie <michael.christie@oracle.com>]

On 12/12/22 10:17 AM, John Garry wrote:
> 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.

I did the WARN_ON_ONCE.