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

[Kevin Wolf]

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 <address@hidden>
> ---
>  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