Re: [PATCH 6/6] scsi-disk: Check for supported commands

[Kevin Wolf <kwolf@redhat.com>]

Am 22.07.2011 16:51, schrieb Hannes Reinecke:
> Not every command is support for any device type. This patch adds
> a check for rejecting unsupported commands.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

We do emulate SEEK (6), but it's not in your scsi_cmd_table at all.

> ---
>  hw/scsi-disk.c |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 103 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index ae2c157..8ad90c0 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -361,13 +361,107 @@ static int scsi_get_sense(SCSIRequest *req, uint8_t *outbuf, int len)
>      return scsi_build_sense(s->sense, outbuf, len, len > 14);
>  }
>  
> +#define GENERIC_CMD (uint32_t)0xFFFFFFFF
> +#define DISK_CMD (1u << TYPE_DISK)
> +#define TAPE_CMD (1u << TYPE_TAPE)
> +#define PRINTER_CMD (1u << TYPE_PRINTER)
> +#define PROCESSOR_CMD (1u << TYPE_PROCESSOR)
> +#define WORM_CMD (1u << TYPE_WORM)
> +#define ROM_CMD (1u << TYPE_ROM)
> +#define SCANNER_CMD (1u << TYPE_SCANNER)
> +#define MOD_CMD (1u << TYPE_MOD)
> +#define MEDIUM_CHANGER_CMD (1u << TYPE_MEDIUM_CHANGER)
> +#define ARRAY_CMD (1u << TYPE_STORAGE_ARRAY)
> +#define ENCLOSURE_CMD (1u << TYPE_ENCLOSURE)
> +#define RBC_CMD (1u << TYPE_RBC)
> +#define OSD_CMD (1u << TYPE_OSD)
> +
> +#define NO_ROM_CMD (GENERIC_CMD | ~ROM_CMD)
> +
> +uint32_t scsi_cmd_table[0x100] = {
> +    [TEST_UNIT_READY]           = GENERIC_CMD,
> +    [REWIND]                    = TAPE_CMD,
> +    [REQUEST_SENSE]             = GENERIC_CMD,
> +    [FORMAT_UNIT]               = DISK_CMD|ROM_CMD,
> +    [READ_BLOCK_LIMITS]         = TAPE_CMD,
> +    [REASSIGN_BLOCKS]           = DISK_CMD|WORM_CMD|MOD_CMD,
> +    [READ_6]                    = DISK_CMD|TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,

My spec says that MMC doesn't support READ(6)

> +    [WRITE_6]                   = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD,
> +    [READ_REVERSE]              = TAPE_CMD,
> +    [WRITE_FILEMARKS]           = TAPE_CMD,
> +    [SPACE]                     = TAPE_CMD,
> +    [INQUIRY]                   = GENERIC_CMD,
> +    [MODE_SELECT]               = GENERIC_CMD,
> +    [RESERVE]                   = TAPE_CMD|PRINTER_CMD,
> +    [RELEASE]                   = TAPE_CMD|PRINTER_CMD,

What's the reason for allowing this for tape, but not e.g. for disks?
It's marked as obsolete for both (and optional for quite a few other types)

> +    [ERASE]                     = TAPE_CMD,
> +    [MODE_SENSE]                = GENERIC_CMD,
> +    [START_STOP]                = GENERIC_CMD,
> +    [RECEIVE_DIAGNOSTIC]        = GENERIC_CMD,
> +    [SEND_DIAGNOSTIC]           = GENERIC_CMD,
> +    [ALLOW_MEDIUM_REMOVAL]      = GENERIC_CMD,
> +    [READ_CAPACITY_10]          = DISK_CMD|WORM_CMD|MOD_CMD,

ROM_CMD, too

> +    [READ_10]                   = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> +    [WRITE_10]                  = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> +    [SEEK_10]                   = TAPE_CMD|WORM_CMD|ROM_CMD|MOD_CMD,

Tape doesn't have SEEK(10) in the spec.

> +    [WRITE_VERIFY_10]           = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> +    [VERIFY_10]                 = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD,
> +    [READ_POSITION]             = TAPE_CMD,
> +    [SYNCHRONIZE_CACHE]         = DISK_CMD|WORM_CMD|ROM_CMD|MOD_CMD|RBC_CMD,
> +    [WRITE_BUFFER]              = GENERIC_CMD,
> +    [READ_BUFFER]               = GENERIC_CMD,
> +    [READ_LONG_10]              = DISK_CMD|WORM_CMD|MOD_CMD,
> +    [WRITE_LONG_10]             = DISK_CMD|WORM_CMD|MOD_CMD,
> +    [WRITE_SAME_10]             = DISK_CMD,
> +    [UNMAP]                     = DISK_CMD,
> +    [READ_TOC]                  = ROM_CMD,
> +    [REPORT_DENSITY_SUPPORT]    = TAPE_CMD,
> +    [GET_CONFIGURATION]         = ROM_CMD,
> +    [LOG_SELECT]                = GENERIC_CMD,
> +    [LOG_SENSE]                 = GENERIC_CMD,
> +    [MODE_SELECT_10]            = GENERIC_CMD,
> +    [RESERVE_10]                = PRINTER_CMD,
> +    [RELEASE_10]                = PRINTER_CMD,
> +    [MODE_SENSE_10]             = GENERIC_CMD,
> +    [PERSISTENT_RESERVE_IN]     = GENERIC_CMD,
> +    [PERSISTENT_RESERVE_OUT]    = GENERIC_CMD,
> +    [VARLENGTH_CDB]             = OSD_CMD,
> +    [WRITE_FILEMARKS_16]        = TAPE_CMD,
> +    [ATA_PASSTHROUGH]           = DISK_CMD|ROM_CMD|RBC_CMD,
> +    [READ_16]                   = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> +    [WRITE_16]                  = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> +    [WRITE_VERIFY_16]           = DISK_CMD|WORM_CMD|MOD_CMD|RBC_CMD,
> +    [SYNCHRONIZE_CACHE_16]      = DISK_CMD|TAPE_CMD|WORM_CMD|MOD_CMD|RBC_CMD,

Tape doesn't have this.

> +    [LOCATE_16]                 = TAPE_CMD,
> +    [WRITE_SAME_16]             = DISK_CMD|TAPE_CMD,

Again not for tape in my spec.

Kevin