Re: [Qemu-devel] [PATCH 3/4] scsi-disk: fix changeable values returned by the MODE SENSE command

[Kevin Wolf <kwolf@redhat.com>]

Am 02.08.2010 17:31, schrieb Bernhard Kohl:
> If the page control (PC) field in the MODE SENSE command defines Changeable
> Values to be returned in the mode pages, don't return any mode page as there
> is no support to change any values.
> 
> Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
> ---
>  hw/scsi-disk.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 927df54..26f7345 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -604,13 +604,15 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
>      uint64_t nb_sectors;
> -    int page, dbd, buflen;
> +    int page, dbd, buflen, page_control;
>      uint8_t *p;
>      uint8_t dev_specific_param;
>  
>      dbd = req->cmd.buf[1]  & 0x8;
>      page = req->cmd.buf[2] & 0x3f;
> -    DPRINTF("Mode Sense (page %d, len %zd)\n", page, req->cmd.xfer);
> +    page_control = (req->cmd.buf[2] & 0xc0) >> 6;
> +    DPRINTF("Mode Sense(%d) (page %d, len %d, page_control %d)\n",
> +        (req->cmd.buf[0] == MODE_SENSE) ? 6 : 10, page, len, page_control);
>      memset(outbuf, 0, req->cmd.xfer);
>      p = outbuf;
>  
> @@ -654,7 +656,8 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>          p += 8;
>      }
>  
> -    switch (page) {
> +    /* Don't return pages if Changeable Values (1) are requested. */
> +    if (page_control != 1) switch (page) {

Coding style (missing braces, and switch belongs on its own line).

>      case 0x04:
>      case 0x05:
>      case 0x08:

I don't think this is enough. Wouldn't this still let the command return
successfully? In fact we need it to fail:

"If the logical unit does not implement changeable parameters mode pages
and the device server receives a MODE SENSE command with 01b in the PC
field, then the command shall be terminated with CHECK CONDITION status,
with the sense key set to ILLEGAL REQUEST, and the additional sense code
set to INVALID FIELD IN CDB."

This should do it if I'm not mistaken:

if (page_control == 0x01) {
    return -1;
}

Kevin