Patchwork [10/16] scsi: move request parsing to common code

login
register
mail settings
Submitter Paolo Bonzini
Date Aug. 3, 2011, 8:49 a.m.
Message ID <1312361359-15445-11-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/108069/
State New
Headers show

Comments

Paolo Bonzini - Aug. 3, 2011, 8:49 a.m.
Also introduce the first occurrence of "independent" SCSIReqOps,
to handle invalid commands in common code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c     |   53 ++++++++++++++++++++++++++++++++++++++---------------
 hw/scsi-disk.c    |    5 -----
 hw/scsi-generic.c |    9 ---------
 hw/scsi.h         |    1 -
 4 files changed, 38 insertions(+), 30 deletions(-)
Peter Maydell - Aug. 12, 2011, 4:12 p.m.
On 3 August 2011 09:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
> @@ -157,8 +172,22 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
>                           uint8_t *buf, void *hba_private)
>  {
>     SCSIRequest *req;
> -    req = d->info->alloc_req(d, tag, lun, hba_private);
> -    memcpy(req->cmd.buf, buf, 16);
> +    SCSICommand cmd;
> +
> +    if (scsi_req_parse(&cmd, d, buf) != 0) {
> +        trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]);
> +        req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private);
> +    } else {
> +        trace_scsi_req_parsed(d->id, lun, tag, buf[0],
> +                              cmd.mode, cmd.xfer);
> +        if (req->cmd.lba != -1) {
> +            trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0],
> +                                      cmd.lba);
> +        }
> +        req = d->info->alloc_req(d, tag, lun, hba_private);
> +    }
> +
> +    req->cmd = cmd;
>     return req;
>  }

This patch makes current master fail to compile with optimisation on:

gcc complains:
hw/scsi-bus.c: In function ‘scsi_req_new’:
hw/scsi-bus.c:375: error: ‘req’ may be used uninitialized in this function

because in the 'else' clause we look at req->cmd.lba before we've
called alloc_req().

My guess is that the tracing should just be moved down to after the
allocation?

-- PMM
Peter Maydell - Aug. 12, 2011, 4:55 p.m.
On 3 August 2011 09:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
> @@ -157,8 +172,22 @@ SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
>                           uint8_t *buf, void *hba_private)
>  {
>     SCSIRequest *req;
> -    req = d->info->alloc_req(d, tag, lun, hba_private);
> -    memcpy(req->cmd.buf, buf, 16);
> +    SCSICommand cmd;
> +
> +    if (scsi_req_parse(&cmd, d, buf) != 0) {
> +        trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]);
> +        req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private);
> +    } else {
> +        trace_scsi_req_parsed(d->id, lun, tag, buf[0],
> +                              cmd.mode, cmd.xfer);
> +        if (req->cmd.lba != -1) {
> +            trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0],
> +                                      cmd.lba);
> +        }
> +        req = d->info->alloc_req(d, tag, lun, hba_private);
> +    }
> +
> +    req->cmd = cmd;
>     return req;
>  }

Does it still make sense to set req->cmd to cmd (and to look at cmd
at all) in the case where scsi_req_parse() failed and might not have
actually initialised all of cmd? For instance the tracing code (added
to scsi_req_new() after this patch) looks at cmd.buf[] based on the
value of buf[0], which seems kind of fragile to me.

-- PMM
Paolo Bonzini - Aug. 12, 2011, 5:05 p.m.
On 08/12/2011 06:12 PM, Peter Maydell wrote:
> This patch makes current master fail to compile with optimisation on:
>
> gcc complains:
> hw/scsi-bus.c: In function ‘scsi_req_new’:
> hw/scsi-bus.c:375: error: ‘req’ may be used uninitialized in this function
>
> because in the 'else' clause we look at req->cmd.lba before we've
> called alloc_req().
>
> My guess is that the tracing should just be moved down to after the
> allocation?

You can also use cmd.lba instead of req->cmd.lba.

I guess the failure depends on the compiler version and tracing options.

Paolo
Paolo Bonzini - Aug. 12, 2011, 5:11 p.m.
On 08/12/2011 06:55 PM, Peter Maydell wrote:
> Does it still make sense to set req->cmd to cmd (and to look at cmd
> at all) in the case where scsi_req_parse() failed and might not have
> actually initialised all of cmd? For instance the tracing code (added
> to scsi_req_new() after this patch) looks at cmd.buf[] based on the
> value of buf[0], which seems kind of fragile to me.

At the point tracing is reached, we know that cmd.buf[] has been 
initialized.  But you're right that it is at least not tidy.

We know that the size of the cdb is 16 (it is always like that, and we 
can make it a requirement).  So we can copy it to cmd->buf before 
knowing cmd->len, at the beginning of scsi_req_parse.  We can also zero 
unconditionally len/xfer/mode (plus set lba to -1) in case the parsing 
fails.

Paolo

Patch

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 07d8704..01dbe02 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -7,6 +7,7 @@ 
 #include "trace.h"
 
 static char *scsibus_get_fw_dev_path(DeviceState *dev);
+static int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf);
 static int scsi_build_sense(uint8_t *in_buf, int in_len,
                             uint8_t *buf, int len, bool fixed);
 
@@ -134,6 +135,20 @@  int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
     return res;
 }
 
+/* SCSIReqOps implementation for invalid commands.  */
+
+static int32_t scsi_invalid_command(SCSIRequest *req, uint8_t *buf)
+{
+    scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE));
+    scsi_req_complete(req, CHECK_CONDITION);
+    return 0;
+}
+
+struct SCSIReqOps reqops_invalid_opcode = {
+    .size         = sizeof(SCSIRequest),
+    .send_command = scsi_invalid_command
+};
+
 SCSIRequest *scsi_req_alloc(SCSIReqOps *reqops, SCSIDevice *d, uint32_t tag,
                             uint32_t lun, void *hba_private)
 {
@@ -157,8 +172,22 @@  SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
                           uint8_t *buf, void *hba_private)
 {
     SCSIRequest *req;
-    req = d->info->alloc_req(d, tag, lun, hba_private);
-    memcpy(req->cmd.buf, buf, 16);
+    SCSICommand cmd;
+
+    if (scsi_req_parse(&cmd, d, buf) != 0) {
+        trace_scsi_req_parse_bad(d->id, lun, tag, buf[0]);
+        req = scsi_req_alloc(&reqops_invalid_opcode, d, tag, lun, hba_private);
+    } else {
+        trace_scsi_req_parsed(d->id, lun, tag, buf[0],
+                              cmd.mode, cmd.xfer);
+        if (req->cmd.lba != -1) {
+            trace_scsi_req_parsed_lba(d->id, lun, tag, buf[0],
+                                      cmd.lba);
+        }
+        req = d->info->alloc_req(d, tag, lun, hba_private);
+    }
+
+    req->cmd = cmd;
     return req;
 }
 
@@ -426,27 +455,21 @@  static uint64_t scsi_cmd_lba(SCSICommand *cmd)
     return lba;
 }
 
-int scsi_req_parse(SCSIRequest *req, uint8_t *buf)
+int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
     int rc;
 
-    if (req->dev->type == TYPE_TAPE) {
-        rc = scsi_req_stream_length(&req->cmd, req->dev, buf);
+    if (dev->type == TYPE_TAPE) {
+        rc = scsi_req_stream_length(cmd, dev, buf);
     } else {
-        rc = scsi_req_length(&req->cmd, req->dev, buf);
+        rc = scsi_req_length(cmd, dev, buf);
     }
     if (rc != 0)
         return rc;
 
-    assert(buf == req->cmd.buf);
-    scsi_cmd_xfer_mode(&req->cmd);
-    req->cmd.lba = scsi_cmd_lba(&req->cmd);
-    trace_scsi_req_parsed(req->dev->id, req->lun, req->tag, buf[0],
-                          req->cmd.mode, req->cmd.xfer);
-    if (req->cmd.lba != -1) {
-        trace_scsi_req_parsed_lba(req->dev->id, req->lun, req->tag, buf[0],
-                              req->cmd.lba);
-    }
+    memcpy(cmd->buf, buf, cmd->len);
+    scsi_cmd_xfer_mode(cmd);
+    cmd->lba = scsi_cmd_lba(cmd);
     return 0;
 }
 
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index ceb0e0a..64aa141 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -974,11 +974,6 @@  static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
     outbuf = (uint8_t *)r->iov.iov_base;
     DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", req->lun, req->tag, buf[0]);
 
-    if (scsi_req_parse(&r->req, buf) != 0) {
-        BADF("Unsupported command length, command %x\n", command);
-        scsi_check_condition(r, SENSE_CODE(INVALID_OPCODE));
-        return 0;
-    }
 #ifdef DEBUG_SCSI
     {
         int i;
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 699831a..e91c32e 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -84,10 +84,6 @@  static void scsi_command_complete(void *opaque, int ret)
         case -EDOM:
             status = TASK_SET_FULL;
             break;
-        case -EINVAL:
-            status = CHECK_CONDITION;
-            scsi_req_build_sense(&r->req, SENSE_CODE(INVALID_FIELD));
-            break;
         case -ENOMEM:
             status = CHECK_CONDITION;
             scsi_req_build_sense(&r->req, SENSE_CODE(TARGET_FAILURE));
@@ -298,11 +294,6 @@  static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
         return 0;
     }
 
-    if (-1 == scsi_req_parse(&r->req, cmd)) {
-        BADF("Unsupported command length, command %x\n", cmd[0]);
-        scsi_command_complete(r, -EINVAL);
-        return 0;
-    }
     scsi_req_fixup(&r->req);
 
     DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", lun, tag,
diff --git a/hw/scsi.h b/hw/scsi.h
index 5a1bbe2..dec814a 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -165,7 +165,6 @@  SCSIRequest *scsi_req_ref(SCSIRequest *req);
 void scsi_req_unref(SCSIRequest *req);
 
 void scsi_req_build_sense(SCSIRequest *req, SCSISense sense);
-int scsi_req_parse(SCSIRequest *req, uint8_t *buf);
 void scsi_req_print(SCSIRequest *req);
 void scsi_req_continue(SCSIRequest *req);
 void scsi_req_data(SCSIRequest *req, int len);