On 2022-04-08 10:55, Bart Van Assche wrote:
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.
The definition of scsi_send_eh_cmnd() is broken, obviously it doesn't
change the cdb of the passed argument. However I don't want to use
the const incorrectness of the existing code to avoid const in
the code I added. So retrofitting needs casts.
-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?
Because I don't want to change that function's interface, just point out
how it is broken.
@@ -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?
Yes, probably a scsi_cmnd_get_changeable_cdb() function which is safe as
long as the new cdb_len <= the existing cdb_len . I think it's the only
occasion I did that due to the cdb[1] overwrite in the lun_in_cdb case
(i.e. SCSI-2 SPI).
+/* 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?
Well that was the way it was before the surgery performed by Christoph.
If reducing the size of the scsi_cmnd structure by another 16 bytes
is that important, it can be easily done. My "long cdb" side of the
union takes 16 bytes currently (12 on a 32 bit machine).
IMO there should be comments added to scsi_cmnd.h to stress an object
of that type is always preceded (in memory) by a struct request object.
They are created as a pair, and are destroyed (freed, destructed) as
a pair.
Doug Gilbert
