Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

From: John Garry
Date: Fri May 19 2023 - 12:06:58 EST


On 18/05/2023 20:54, Bart Van Assche wrote:

Further to that, I dislike how we pass a pointer to this local sshdr structure. I would prefer if scsi_execute_cmd() could kmalloc() the mem for these buffers and the callers could handle free'ing them - I can put together a patch for that, to see what people think.

sizeof(struct scsi_sense_hdr) = 8. Using kmalloc() to allocate an eight byte data structure sounds like overkill to me. Additionally, making scsi_execute_cmd() allocate struct scsi_sense_hdr and letting the callers free that data structure will make it harder to review whether or not any memory leaks are triggered. No such review is necessary if the scsi_execute_cmd() caller allocates that data structure on the stack.

Sure, what I describe is ideal, but I still just dislike passing both sensebuf and hdr into scsi_execute_cmd(). The semantics of how scsi_execute_cmd() treats them is vague.

Thanks,
John