Patchwork [2/4] scsi-disk: fix the mode data header returned by the MODE SENSE(10) command

login
register
mail settings
Submitter Bernhard Kohl
Date Aug. 2, 2010, 3:31 p.m.
Message ID <1280763089-11829-3-git-send-email-bernhard.kohl@nsn.com>
Download mbox | patch
Permalink /patch/60548/
State New
Headers show

Comments

Bernhard Kohl - Aug. 2, 2010, 3:31 p.m.
The header for the  MODE SENSE(10) command is 8 bytes long.

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-disk.c |   35 ++++++++++++++++++++++++++++-------
 1 files changed, 28 insertions(+), 7 deletions(-)
Kevin Wolf - Aug. 16, 2010, 5:02 p.m.
Am 02.08.2010 17:31, schrieb Bernhard Kohl:
> The header for the  MODE SENSE(10) command is 8 bytes long.
> 
> Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
> ---
>  hw/scsi-disk.c |   35 ++++++++++++++++++++++++++++-------
>  1 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 57439f4..927df54 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -606,6 +606,7 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>      uint64_t nb_sectors;
>      int page, dbd, buflen;
>      uint8_t *p;
> +    uint8_t dev_specific_param;
>  
>      dbd = req->cmd.buf[1]  & 0x8;
>      page = req->cmd.buf[2] & 0x3f;
> @@ -613,16 +614,31 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>      memset(outbuf, 0, req->cmd.xfer);
>      p = outbuf;
>  
> -    p[1] = 0; /* Default media type.  */
> -    p[3] = 0; /* Block descriptor length.  */
> -    if (bdrv_is_read_only(s->bs)) {
> -        p[2] = 0x80; /* Readonly.  */
> +    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM ||
> +        bdrv_is_read_only(s->bs)) {

This looks like a mismerge. The check for CDROMs was removed when they
became read-only by definition. Please don't reintroduce it.

> +        dev_specific_param = 0x80; /* Readonly.  */
> +    } else {
> +        dev_specific_param = 0x00;
> +    }
> +
> +    if (req->cmd.buf[0] == MODE_SENSE) {
> +        p[1] = 0; /* Default media type.  */
> +        p[2] = dev_specific_param;
> +        p[3] = 0; /* Block descriptor length.  */
> +        p += 4;
> +    } else { /* MODE_SENSE_10 */
> +        p[2] = 0; /* Default media type.  */
> +        p[3] = dev_specific_param;
> +        p[6] = p[7] = 0; /* Block descriptor length.  */
> +        p += 8;
>      }
> -    p += 4;
>  
>      bdrv_get_geometry(s->bs, &nb_sectors);
>      if ((~dbd) & nb_sectors) {
> -        outbuf[3] = 8; /* Block descriptor length  */
> +        if (req->cmd.buf[0] == MODE_SENSE)
> +            outbuf[3] = 8; /* Block descriptor length  */
> +        else /* MODE_SENSE_10 */
> +            outbuf[7] = 8; /* Block descriptor length  */

Please add curly braces here (see CODING_STYLE).

Kevin
Bernhard Kohl - Aug. 27, 2010, 3:24 p.m.
Am 16.08.2010 19:02, schrieb ext Kevin Wolf:
>
> > +    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM ||
> > +        bdrv_is_read_only(s->bs)) {
>
> This looks like a mismerge. The check for CDROMs was removed when they
> became read-only by definition. Please don't reintroduce it.
>
OK, I will remove that check in v2.
>
> > +        if (req->cmd.buf[0] == MODE_SENSE)
> > +            outbuf[3] = 8; /* Block descriptor length  */
> > +        else /* MODE_SENSE_10 */
> > +            outbuf[7] = 8; /* Block descriptor length  */
>
> Please add curly braces here (see CODING_STYLE).
>
OK, I will add curly braces in v2.

Bernhard

Patch

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 57439f4..927df54 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -606,6 +606,7 @@  static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
     uint64_t nb_sectors;
     int page, dbd, buflen;
     uint8_t *p;
+    uint8_t dev_specific_param;
 
     dbd = req->cmd.buf[1]  & 0x8;
     page = req->cmd.buf[2] & 0x3f;
@@ -613,16 +614,31 @@  static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
     memset(outbuf, 0, req->cmd.xfer);
     p = outbuf;
 
-    p[1] = 0; /* Default media type.  */
-    p[3] = 0; /* Block descriptor length.  */
-    if (bdrv_is_read_only(s->bs)) {
-        p[2] = 0x80; /* Readonly.  */
+    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM ||
+        bdrv_is_read_only(s->bs)) {
+        dev_specific_param = 0x80; /* Readonly.  */
+    } else {
+        dev_specific_param = 0x00;
+    }
+
+    if (req->cmd.buf[0] == MODE_SENSE) {
+        p[1] = 0; /* Default media type.  */
+        p[2] = dev_specific_param;
+        p[3] = 0; /* Block descriptor length.  */
+        p += 4;
+    } else { /* MODE_SENSE_10 */
+        p[2] = 0; /* Default media type.  */
+        p[3] = dev_specific_param;
+        p[6] = p[7] = 0; /* Block descriptor length.  */
+        p += 8;
     }
-    p += 4;
 
     bdrv_get_geometry(s->bs, &nb_sectors);
     if ((~dbd) & nb_sectors) {
-        outbuf[3] = 8; /* Block descriptor length  */
+        if (req->cmd.buf[0] == MODE_SENSE)
+            outbuf[3] = 8; /* Block descriptor length  */
+        else /* MODE_SENSE_10 */
+            outbuf[7] = 8; /* Block descriptor length  */
         nb_sectors /= s->cluster_size;
         nb_sectors--;
         if (nb_sectors > 0xffffff)
@@ -652,7 +668,12 @@  static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
     }
 
     buflen = p - outbuf;
-    outbuf[0] = buflen - 1;
+    if (req->cmd.buf[0] == MODE_SENSE) {
+        outbuf[0] = buflen - 1;
+    } else { /* MODE_SENSE_10 */
+        outbuf[0] = ((buflen - 1) >> 8) & 0xff;
+        outbuf[1] = (buflen - 1) & 0xff;
+    }
     if (buflen > req->cmd.xfer)
         buflen = req->cmd.xfer;
     return buflen;