Re: [Qemu-devel] [PATCH 1/3] scsi: Sanitize command definitions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/22/2011 04:07 PM, Markus Armbruster wrote:
Hannes Reinecke<hare@xxxxxxx>  writes:

Adding some missing command definitions and update the existing ones.
No functional change.

Add: LOCATE_10, UNMAP, VARLENGTH_CDB, WRITE_FILEMARKS_16, EXTENDED_COPY,
ATA_PASSTHROUGH, ACCESS_CONTROL_IN, ACCESS_CONTROL_OUT,
COMPARE_AND_WRITE, VERIFY_16, SYNCHRONIZE_CACHE_16, LOCATE_16, ERASE_16,
WRITE_LONG_16, VERIFY_12, READ_DEFECT_DATA_12.

Rename: READ_CAPACITY, WRITE_VERIFY, VERIFY, READ_LONG, WRITE_LONG,
WRITE_SAME to&_10.

Delete: WRITE_LONG_2

OK.


Signed-off-by: Hannes Reinecke<hare@xxxxxxx>
---
  hw/scsi-bus.c     |   71 ++++++++++++++++++++++++++++++----------------------
  hw/scsi-defs.h    |   60 ++++++++++++++++++++++++++++----------------
  hw/scsi-disk.c    |   29 ++++++++-------------
  hw/scsi-generic.c |    2 +-
  4 files changed, 91 insertions(+), 71 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 8b1a412..24a1298 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -232,24 +232,24 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
      case RELEASE:
      case ERASE:
      case ALLOW_MEDIUM_REMOVAL:
-    case VERIFY:
+    case VERIFY_10:
      case SEEK_10:
      case SYNCHRONIZE_CACHE:
      case LOCK_UNLOCK_CACHE:
      case LOAD_UNLOAD:
      case SET_CD_SPEED:
      case SET_LIMITS:
-    case WRITE_LONG:
+    case WRITE_LONG_10:
      case MOVE_MEDIUM:
      case UPDATE_BLOCK:
          req->cmd.xfer = 0;
          break;
      case MODE_SENSE:
          break;
-    case WRITE_SAME:
+    case WRITE_SAME_10:
          req->cmd.xfer = 1;
          break;
-    case READ_CAPACITY:
+    case READ_CAPACITY_10:
          req->cmd.xfer = 8;
          break;
      case READ_BLOCK_LIMITS:
@@ -265,7 +265,7 @@ static int scsi_req_length(SCSIRequest *req, uint8_t *cmd)
          req->cmd.xfer *= 8;
          break;
      case WRITE_10:
-    case WRITE_VERIFY:
+    case WRITE_VERIFY_10:
      case WRITE_6:
      case WRITE_12:
      case WRITE_VERIFY_12:
@@ -325,7 +325,7 @@ static void scsi_req_xfer_mode(SCSIRequest *req)
      switch (req->cmd.buf[0]) {
      case WRITE_6:
      case WRITE_10:
-    case WRITE_VERIFY:
+    case WRITE_VERIFY_10:
      case WRITE_12:
      case WRITE_VERIFY_12:
      case WRITE_16:
@@ -345,15 +345,13 @@ static void scsi_req_xfer_mode(SCSIRequest *req)
      case SEARCH_HIGH:
      case SEARCH_LOW:
      case UPDATE_BLOCK:
-    case WRITE_LONG:
-    case WRITE_SAME:
+    case WRITE_LONG_10:
+    case WRITE_SAME_10:
      case SEARCH_HIGH_12:
      case SEARCH_EQUAL_12:
      case SEARCH_LOW_12:
-    case SET_WINDOW:

I figure this is "no functional change" because all users of this
function reject SET_WINDOW elsewhere.

Yes, you are correct. Will be doing another patch with removing references to obsolete commands SET_WINDOW and WRITE_LONG_2.

      case MEDIUM_SCAN:
      case SEND_VOLUME_TAG:
-    case WRITE_LONG_2:

Same here.

Indeed.

      case PERSISTENT_RESERVE_OUT:
      case MAINTENANCE_OUT:
          req->cmd.mode = SCSI_XFER_TO_DEV;
@@ -517,8 +515,7 @@ static const char *scsi_command_name(uint8_t cmd)
  {
      static const char *names[] = {
          [ TEST_UNIT_READY          ] = "TEST_UNIT_READY",
-        [ REZERO_UNIT              ] = "REZERO_UNIT",
-        /* REWIND and REZERO_UNIT use the same operation code */
+        [ REWIND                   ] = "REWIND",

"No functional change" because it's used only for scsi_req_print(),
which is unused.

See above.

          [ REQUEST_SENSE            ] = "REQUEST_SENSE",
          [ FORMAT_UNIT              ] = "FORMAT_UNIT",
          [ READ_BLOCK_LIMITS        ] = "READ_BLOCK_LIMITS",
@@ -543,14 +540,13 @@ static const char *scsi_command_name(uint8_t cmd)
          [ RECEIVE_DIAGNOSTIC       ] = "RECEIVE_DIAGNOSTIC",
          [ SEND_DIAGNOSTIC          ] = "SEND_DIAGNOSTIC",
          [ ALLOW_MEDIUM_REMOVAL     ] = "ALLOW_MEDIUM_REMOVAL",
-
          [ SET_WINDOW               ] = "SET_WINDOW",
-        [ READ_CAPACITY            ] = "READ_CAPACITY",
+        [ READ_CAPACITY_10         ] = "READ_CAPACITY_10",
          [ READ_10                  ] = "READ_10",
          [ WRITE_10                 ] = "WRITE_10",
          [ SEEK_10                  ] = "SEEK_10",

New LOCATE_10 missing.

OK.

-        [ WRITE_VERIFY             ] = "WRITE_VERIFY",
-        [ VERIFY                   ] = "VERIFY",
+        [ WRITE_VERIFY_10          ] = "WRITE_VERIFY_10",
+        [ VERIFY_10                ] = "VERIFY_10",
          [ SEARCH_HIGH              ] = "SEARCH_HIGH",
          [ SEARCH_EQUAL             ] = "SEARCH_EQUAL",
          [ SEARCH_LOW               ] = "SEARCH_LOW",
@@ -566,11 +562,14 @@ static const char *scsi_command_name(uint8_t cmd)
          [ WRITE_BUFFER             ] = "WRITE_BUFFER",
          [ READ_BUFFER              ] = "READ_BUFFER",
          [ UPDATE_BLOCK             ] = "UPDATE_BLOCK",
-        [ READ_LONG                ] = "READ_LONG",
-        [ WRITE_LONG               ] = "WRITE_LONG",
+        [ READ_LONG_10             ] = "READ_LONG_10",
+        [ WRITE_LONG_10            ] = "WRITE_LONG_10",
          [ CHANGE_DEFINITION        ] = "CHANGE_DEFINITION",
-        [ WRITE_SAME               ] = "WRITE_SAME",
+        [ WRITE_SAME_10            ] = "WRITE_SAME_10",
+        [ UNMAP                    ] = "UNMAP",
          [ READ_TOC                 ] = "READ_TOC",
+        [ REPORT_DENSITY_SUPPORT   ] = "REPORT_DENSITY_SUPPORT",
+        [ GET_CONFIGURATION        ] = "GET_CONFIGURATION",
          [ LOG_SELECT               ] = "LOG_SELECT",
          [ LOG_SENSE                ] = "LOG_SENSE",
          [ MODE_SELECT_10           ] = "MODE_SELECT_10",
@@ -579,27 +578,39 @@ static const char *scsi_command_name(uint8_t cmd)
          [ MODE_SENSE_10            ] = "MODE_SENSE_10",
          [ PERSISTENT_RESERVE_IN    ] = "PERSISTENT_RESERVE_IN",
          [ PERSISTENT_RESERVE_OUT   ] = "PERSISTENT_RESERVE_OUT",

New VARLENGTH_CDB missing.

Intentionally. VARLENGHT_CDB is used for all sorts of things, so
I think we should be printing that one out separately.
Same goes for MAINTENANCE_(IN|OUT) and SERVICE_ACTION_(IN|OUT).

+        [ WRITE_FILEMARKS_16       ] = "WRITE_FILEMARKS_16",
+        [ EXTENDED_COPY            ] = "EXTENDED_COPY",
+        [ ATA_PASSTHROUGH          ] = "ATA_PASSTHROUGH",
+        [ ACCESS_CONTROL_IN        ] = "ACCESS_CONTROL_IN",
+        [ ACCESS_CONTROL_OUT       ] = "ACCESS_CONTROL_OUT",
+        [ READ_16                  ] = "READ_16",
+        [ COMPARE_AND_WRITE        ] = "COMPARE_AND_WRITE",
+        [ WRITE_16                 ] = "WRITE_16",
+        [ WRITE_VERIFY_16          ] = "WRITE_VERIFY_16",
+        [ VERIFY_16                ] = "VERIFY_16",
+        [ SYNCHRONIZE_CACHE_16     ] = "SYNCHRONIZE_CACHE_16",
+        [ LOCATE_16                ] = "LOCATE_16",
+        [ WRITE_SAME_16            ] = "WRITE_SAME_16",
+        [ ERASE_16                 ] = "ERASE_16",
+        [ SERVICE_ACTION_IN        ] = "SERVICE_ACTION_IN",
+        [ WRITE_LONG_16            ] = "WRITE_LONG_16",
+        [ REPORT_LUNS              ] = "REPORT_LUNS",
+        [ BLANK                    ] = "BLANK",
+        [ MAINTENANCE_IN           ] = "MAINTENANCE_IN",
+        [ MAINTENANCE_OUT          ] = "MAINTENANCE_OUT",

Fixes missing WRITE_SAME_16, MAINTENANCE_IN, MAINTENANCE_OUT.

Yes.

          [ MOVE_MEDIUM              ] = "MOVE_MEDIUM",
+        [ LOAD_UNLOAD              ] = "LOAD_UNLOAD",
          [ READ_12                  ] = "READ_12",
          [ WRITE_12                 ] = "WRITE_12",
          [ WRITE_VERIFY_12          ] = "WRITE_VERIFY_12",
+        [ VERIFY_12                ] = "VERIFY_12",
          [ SEARCH_HIGH_12           ] = "SEARCH_HIGH_12",
          [ SEARCH_EQUAL_12          ] = "SEARCH_EQUAL_12",
          [ SEARCH_LOW_12            ] = "SEARCH_LOW_12",
          [ READ_ELEMENT_STATUS      ] = "READ_ELEMENT_STATUS",
          [ SEND_VOLUME_TAG          ] = "SEND_VOLUME_TAG",
-        [ WRITE_LONG_2             ] = "WRITE_LONG_2",
-
-        [ REPORT_DENSITY_SUPPORT   ] = "REPORT_DENSITY_SUPPORT",
-        [ GET_CONFIGURATION        ] = "GET_CONFIGURATION",
-        [ READ_16                  ] = "READ_16",
-        [ WRITE_16                 ] = "WRITE_16",
-        [ WRITE_VERIFY_16          ] = "WRITE_VERIFY_16",
-        [ SERVICE_ACTION_IN        ] = "SERVICE_ACTION_IN",
-        [ REPORT_LUNS              ] = "REPORT_LUNS",
-        [ LOAD_UNLOAD              ] = "LOAD_UNLOAD",
+        [ READ_DEFECT_DATA         ] = "READ_DEFECT_DATA",

READ_DEFECT_DATA_12!

Yes.

          [ SET_CD_SPEED             ] = "SET_CD_SPEED",
-        [ BLANK                    ] = "BLANK",
      };

      if (cmd>= ARRAY_SIZE(names) || names[cmd] == NULL)
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index 413cce0..458a797 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -26,6 +26,7 @@

  #define TEST_UNIT_READY       0x00
  #define REZERO_UNIT           0x01
+#define REWIND                0x01
  #define REQUEST_SENSE         0x03
  #define FORMAT_UNIT           0x04
  #define READ_BLOCK_LIMITS     0x05
@@ -48,14 +49,14 @@
  #define RECEIVE_DIAGNOSTIC    0x1c
  #define SEND_DIAGNOSTIC       0x1d
  #define ALLOW_MEDIUM_REMOVAL  0x1e
-
  #define SET_WINDOW            0x24
-#define READ_CAPACITY         0x25
+#define READ_CAPACITY_10      0x25
  #define READ_10               0x28
  #define WRITE_10              0x2a
  #define SEEK_10               0x2b
-#define WRITE_VERIFY          0x2e
-#define VERIFY                0x2f
+#define LOCATE_10             0x2b
+#define WRITE_VERIFY_10       0x2e
+#define VERIFY_10             0x2f
  #define SEARCH_HIGH           0x30
  #define SEARCH_EQUAL          0x31
  #define SEARCH_LOW            0x32
@@ -71,11 +72,14 @@
  #define WRITE_BUFFER          0x3b
  #define READ_BUFFER           0x3c
  #define UPDATE_BLOCK          0x3d
-#define READ_LONG             0x3e
-#define WRITE_LONG            0x3f
+#define READ_LONG_10          0x3e
+#define WRITE_LONG_10         0x3f
  #define CHANGE_DEFINITION     0x40
-#define WRITE_SAME            0x41
+#define WRITE_SAME_10         0x41
+#define UNMAP                 0x42
  #define READ_TOC              0x43
+#define REPORT_DENSITY_SUPPORT 0x44
+#define GET_CONFIGURATION     0x46
  #define LOG_SELECT            0x4c
  #define LOG_SENSE             0x4d
  #define MODE_SELECT_10        0x55
@@ -84,32 +88,40 @@
  #define MODE_SENSE_10         0x5a
  #define PERSISTENT_RESERVE_IN 0x5e
  #define PERSISTENT_RESERVE_OUT 0x5f
+#define VARLENGTH_CDB         0x7f
+#define WRITE_FILEMARKS_16    0x80
+#define EXTENDED_COPY         0x83
+#define ATA_PASSTHROUGH       0x85
+#define ACCESS_CONTROL_IN     0x86
+#define ACCESS_CONTROL_OUT    0x87
+#define READ_16               0x88
+#define COMPARE_AND_WRITE     0x89
+#define WRITE_16              0x8a
+#define WRITE_VERIFY_16       0x8e
+#define VERIFY_16             0x8f
+#define SYNCHRONIZE_CACHE_16  0x91
+#define LOCATE_16             0x92
  #define WRITE_SAME_16         0x93
+#define ERASE_16              0x93
+#define SERVICE_ACTION_IN     0x9e
+#define WRITE_LONG_16         0x9f
+#define REPORT_LUNS           0xa0
+#define BLANK                 0xa1
  #define MAINTENANCE_IN        0xa3
  #define MAINTENANCE_OUT       0xa4
  #define MOVE_MEDIUM           0xa5
+#define LOAD_UNLOAD           0xa6
  #define READ_12               0xa8
  #define WRITE_12              0xaa
  #define WRITE_VERIFY_12       0xae
+#define VERIFY_12             0xaf
  #define SEARCH_HIGH_12        0xb0
  #define SEARCH_EQUAL_12       0xb1
  #define SEARCH_LOW_12         0xb2
  #define READ_ELEMENT_STATUS   0xb8
  #define SEND_VOLUME_TAG       0xb6
-#define WRITE_LONG_2          0xea
-
-/* from hw/scsi-generic.c */
-#define REWIND 0x01
-#define REPORT_DENSITY_SUPPORT 0x44
-#define GET_CONFIGURATION 0x46
-#define READ_16 0x88
-#define WRITE_16 0x8a
-#define WRITE_VERIFY_16 0x8e
-#define SERVICE_ACTION_IN 0x9e
-#define REPORT_LUNS 0xa0
-#define LOAD_UNLOAD 0xa6
-#define SET_CD_SPEED 0xbb
-#define BLANK 0xa1
+#define READ_DEFECT_DATA_12   0xb7
+#define SET_CD_SPEED          0xbb

  /*
   *  SAM Status codes
@@ -154,6 +166,7 @@

  #define TYPE_DISK           0x00
  #define TYPE_TAPE           0x01
+#define TYPE_PRINTER        0x02
  #define TYPE_PROCESSOR      0x03    /* HP scanners use this */
  #define TYPE_WORM           0x04    /* Treated as ROM by our system */
  #define TYPE_ROM            0x05
@@ -161,6 +174,9 @@
  #define TYPE_MOD            0x07    /* Magneto-optical disk -
  				     * - treated as TYPE_DISK */
  #define TYPE_MEDIUM_CHANGER 0x08
-#define TYPE_ENCLOSURE	    0x0d    /* Enclosure Services Device */
+#define TYPE_STORAGE_ARRAY  0x0c    /* Storage array device */
+#define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
+#define TYPE_RBC            0x0e    /* Simplified Direct-Access Device */
+#define TYPE_OSD            0x11    /* Object-storage Device */
  #define TYPE_NO_LUN         0x7f

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 05d14ab..2f7e0c9 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -836,7 +836,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
      case TEST_UNIT_READY:
          if (!bdrv_is_inserted(s->bs))
              goto not_ready;
-	break;
+        break;
      case REQUEST_SENSE:
          if (req->cmd.xfer<  4)
              goto illegal_request;
@@ -848,7 +848,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
          buflen = scsi_disk_emulate_inquiry(req, outbuf);
          if (buflen<  0)
              goto illegal_request;
-	break;
+        break;
      case MODE_SENSE:
      case MODE_SENSE_10:
          buflen = scsi_disk_emulate_mode_sense(req, outbuf);
@@ -881,14 +881,14 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
              /* load/eject medium */
              bdrv_eject(s->bs, !(req->cmd.buf[4]&  1));
          }
-	break;
+        break;
      case ALLOW_MEDIUM_REMOVAL:
          bdrv_set_locked(s->bs, req->cmd.buf[4]&  1);
-	break;
+        break;
      case READ_CAPACITY:
          /* The normal LEN field for this command is zero.  */
-	memset(outbuf, 0, 8);
-	bdrv_get_geometry(s->bs,&nb_sectors);
+        memset(outbuf, 0, 8);
+        bdrv_get_geometry(s->bs,&nb_sectors);
          if (!nb_sectors)
              goto not_ready;
          nb_sectors /= s->cluster_size;
@@ -908,7 +908,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
          outbuf[6] = s->cluster_size * 2;
          outbuf[7] = 0;
          buflen = 8;
-	break;
+        break;
      case SYNCHRONIZE_CACHE:
          ret = bdrv_flush(s->bs);
          if (ret<  0) {
@@ -970,13 +970,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
          outbuf[3] = 8;
          buflen = 16;
          break;
-    case VERIFY:
-        break;
-    case REZERO_UNIT:
-        DPRINTF("Rezero Unit\n");
-        if (!bdrv_is_inserted(s->bs)) {
-            goto not_ready;
-        }

Functional change, the commit message lies.  Separate patch?

Matching update to scsi_cmd_table[] missing.

Looks like we could purge REZERO_UNIT elsewhere, too.

+    case VERIFY_10:
          break;
      default:
          scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
@@ -1052,14 +1046,13 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
      case RELEASE_10:
      case START_STOP:
      case ALLOW_MEDIUM_REMOVAL:
-    case READ_CAPACITY:
+    case READ_CAPACITY_10:
      case SYNCHRONIZE_CACHE:
      case READ_TOC:
      case GET_CONFIGURATION:
      case SERVICE_ACTION_IN:
      case REPORT_LUNS:
-    case VERIFY:
-    case REZERO_UNIT:
+    case VERIFY_10:
          rc = scsi_disk_emulate_command(r, outbuf);
          if (rc<  0) {
              return 0;
@@ -1082,7 +1075,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
      case WRITE_10:
      case WRITE_12:
      case WRITE_16:
-    case WRITE_VERIFY:
+    case WRITE_VERIFY_10:
      case WRITE_VERIFY_12:
      case WRITE_VERIFY_16:
          len = r->req.cmd.xfer / s->qdev.blocksize;
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 90345a7..17e83c7 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -406,7 +406,7 @@ static int get_blocksize(BlockDriverState *bdrv)

      memset(cmd, 0, sizeof(cmd));
      memset(buf, 0, sizeof(buf));
-    cmd[0] = READ_CAPACITY;
+    cmd[0] = READ_CAPACITY_10;

      memset(&io_header, 0, sizeof(io_header));
      io_header.interface_id = 'S';
OK, convinced. Will be doing a separate patch for removing the obsolete commands.

Cheers,

Hannes
--
Dr. Hannes Reinecke		      zSeries & Storage
hare@xxxxxxx			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux